-
Notifications
You must be signed in to change notification settings - Fork 653
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
Comments
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 |
@adetunjii I'm okay with you working on it, but we still need to get consensus first we want to build it. |
I do not object to add this argument/value pair. But this internal command could be called by replica and master. |
@valkey-io/core-team Thoughts on this? Not asking for a specific vote, but feel free to 👍 or 👎 . |
This doesn't really solve the problem unless we are going to send every single forward compatibility breaking change as a capability. |
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. |
Capability flag for each new feature is what I had in mind. Version check works too. I don't have a strong opinion. |
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. |
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.) |
It seems we have consensus (or at least majority) on this. I opened a PR.
@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 |
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]>
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: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.
The text was updated successfully, but these errors were encountered: