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

Consider encoding the version of message scheme in the preamble #583

Open
freeekanayaka opened this issue May 30, 2023 · 7 comments
Open
Labels
Feature New feature, not a bug

Comments

@freeekanayaka
Copy link
Contributor

In this commit of canonical/raft#303 we started using only the first 2 bytes to read the message type out of a 64-bit slot in the message preamble, instead of using the full 8 bytes of the slot.

That was done in preparation of using the rest of the slot to store additional information, such has a version for the scheme of a specific message type.

I think sufficient time has now passed that we could start actually encoding the schema version in that space, and be confident that the node on the other side will either read it (and take it into account when decoding), or ignore it and assume that the version is 0.

Thoughts?

@cole-miller
Copy link
Contributor

Would this just be a cleanup, or does it unlock some possibilities that the current message format prevents?

@freeekanayaka
Copy link
Contributor Author

Currently we use the length of the message in bytes to evince its schema version. For example we do that for the RequestVote RPC, see here.

This works well for adding fields, because the total size of an encoded message whose type is using a newer schema version (with added fields) will be higher than the encoded size of the same message type at a lower version schema. If the receiver does not support the new fields it will just read up to the size it supports and mark the message as using the lower version schema. If the receiver does support the new fields, it can use the message size to decide whether the schema version is the higher one or the lower, and set the version field of the decoded message accordingly, along with the other fields.

However this technique is not enough when we want to change a schema version without changing the message size (e.g. replacing or repurposing a field, or similar things).

@freeekanayaka
Copy link
Contributor Author

freeekanayaka commented May 30, 2023

In general I think we should always only add fields, that makes everything easier, so the current approach should be enough. However there might be cases were can't or don't want to do that, and it's good to have a way out for those cases. The change in the PR that I mention was a preparation to be able to support this.

@cole-miller
Copy link
Contributor

In general I think we should always only add fields, that makes everything easier, so the current approach should be enough.

Yeah, this is how I feel. I think we should revisit this change when we have a use-case for it.

@freeekanayaka
Copy link
Contributor Author

In general I think we should always only add fields, that makes everything easier, so the current approach should be enough.

Yeah, this is how I feel. I think we should revisit this change when we have a use-case for it.

Yeah, however unless I'm missing something, the problem I see is that when we'll have a use case, we will not be able to use this system because it won't be in place, so we won't be able to meet the use case. I think that was the reason to start putting it in place in the first place, to allow for future changes.

@cole-miller
Copy link
Contributor

Ah, I see, because we want the support to be in place for a while before making use of it, so that we're justified in assuming that all servers in the cluster will have it?

@freeekanayaka
Copy link
Contributor Author

Ah, I see, because we want the support to be in place for a while before making use of it, so that we're justified in assuming that all servers in the cluster will have it?

Exactly.

@MathieuBordere MathieuBordere added the Feature New feature, not a bug label Jun 12, 2023
@cole-miller cole-miller transferred this issue from canonical/raft Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, not a bug
Projects
None yet
Development

No branches or pull requests

3 participants