Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replicas should indicate their version to their Primaries during handshake #414

Closed
madolson opened this issue May 2, 2024 · 11 comments · Fixed by #554
Closed

Replicas should indicate their version to their Primaries during handshake #414

madolson opened this issue May 2, 2024 · 11 comments · Fixed by #554
Labels
major-decision-approved Major decision approved by TSC team

Comments

@madolson
Copy link
Member

madolson commented May 2, 2024

I've seen it pop up in a few places where it be ideal to know what the version of the replica is we are replicating too, to potentially send something it might understand (and be potentially more compatible). I would suggest we extend the REPLCONF command to also support configuring the version metadata. Something like:

REPLCONF version 7.0.5

The replica would optionally send this after the other capabilities, so that it could gracefully handle the case where the primary does not support it.

I thought about this after seeing, https://github.com/valkey-io/valkey/pull/245/files#r1586619641 (moved to follow-up issue #421), which would allow us to abort if a replica isn't on the same version as us.

@adetunjii
Copy link
Contributor

Hi @madolson. I want to take a look at this issue. I'm not exactly sure if I have to be assigned to an issue before I can work on it, or if I can go ahead and find a fix

@madolson
Copy link
Member Author

madolson commented May 2, 2024

@adetunjii I'm okay with you working on it, but we still need to get consensus first we want to build it.

@madolson madolson added the major-decision-pending Major decision pending by TSC team label May 2, 2024
@hwware
Copy link
Member

hwware commented May 2, 2024

I do not object to add this argument/value pair. But this internal command could be called by replica and master.
Thus I want to suggest if replica can call this command with format "REPLCONF version version-number", and master could call this command as "REPLCONF version" and get replica version number as well.

@madolson
Copy link
Member Author

madolson commented May 3, 2024

@valkey-io/core-team Thoughts on this? Not asking for a specific vote, but feel free to 👍 or 👎 .

@zuiderkwast
Copy link
Contributor

zuiderkwast commented May 3, 2024

In general, I think it's better to report capabilities.

Feature negotiation between two peers = exchange capabilities flags.

For #245 (or its follow-up #421) I think we can use the cluster bus instead. We can just pick an unused bit in clusterMsg as a capability flag.

@madolson
Copy link
Member Author

madolson commented May 3, 2024

Feature negotiation between two peers = exchange capabilities flags.

This doesn't really solve the problem unless we are going to send every single forward compatibility breaking change as a capability.

@PingXie
Copy link
Member

PingXie commented May 3, 2024

I assume the proposed version scheme is meant to track the compatibility of the wire protocol (in terms of new commands/flags/etc)?

I am generally aligned with the version scheme. We can discuss the details over the PR.

Also generally speaking, I don't think a granular compat negotiation protocol like "capability bits" would scale.

@zuiderkwast
Copy link
Contributor

Feature negotiation between two peers = exchange capabilities flags.

This doesn't really solve the problem unless we are going to send every single breaking change as a capability.

Capability flag for each new feature is what I had in mind. Version check works too. I don't have a strong opinion.

@soloestoy
Copy link
Member

I like this idea, primary and replicas should exchange their version. But I don't think we should abort the replication if the replica's version is not same with primary, that would block upgrade and rollback via replication.

@zuiderkwast
Copy link
Contributor

Version might be good to have for INFO fields or to use as capas for e.g. new types or such. But there are more aspects than version, like config and modules, that affect what a replica is capable of.

For the #421 issue, what the primary needs to know is that the replica can accept replication of CLUSTER SETSLOT. I believe this should only be enabled if the replica is >= 8.0 and in cluster mode. So I'm thinking that maybe a capa is needed, or that we use a bit in the cluster bus to signal this capa.

Is it possible for a replica to be in non-cluster node and replicate from a cluster primary? (I'm thinking about fake-replicas like valkey-cli and others special scenarios.)

@zuiderkwast zuiderkwast added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels May 26, 2024
@zuiderkwast
Copy link
Contributor

It seems we have consensus (or at least majority) on this.

I opened a PR.

I do not object to add this argument/value pair. But this internal command could be called by replica and master.
Thus I want to suggest if replica can call this command with format "REPLCONF version version-number", and master could call this command as "REPLCONF version" and get replica version number as well.

@hwware The replica sends all its info using REPLCONF during handshake (listening-port, ip-address, capa). There is no need for the primary to ask the replica for this later. It seems logical to do the same with REPLCONF VERSION x.y.z.

zuiderkwast added a commit that referenced this issue May 27, 2024
The replica sends its version when initiating replication, in
pipeline with other REPLCONF commands.

The primary stores it in the client struct. Other fields are made
smaller to avoid making the client struct consume more memory.

Fixes #414.

---------

Signed-off-by: Viktor Söderqvist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Major decision approved by TSC team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants