-
Notifications
You must be signed in to change notification settings - Fork 560
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
Prepare 0.24 for rolling upgrade to 0.25 #5389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some minor comments. Did you run a benchmark with this version?
private final NamespaceImpl namespace; | ||
|
||
FallbackNamespace(final NamespaceImpl fallback, final NamespaceImpl namespace) { | ||
this.fallback = fallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name fallback is a bit misleading as it uses fallback as default for serializing. Can we give more meaningful names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names were requested by the reviewer in the last review #5042 (comment). The initial names were legacy
and compatible
. Can you suggest alternatives that are more meaningful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it make sense to use "fallback" in 0.25 because it is actually using it as a fallback. But here fallback is the default. legacy
and compatible
sounds good to me. Or just switch the places - current namespace is the new fallback.
Thanks for the review. Yes, the benchmark has been running under the |
Thanks for the feedback @deepthidevaki. The latest commit changes the namespace names to compatible and legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
dffba7b
to
3f13177
Compare
bors r+ |
Build succeeded: |
Description
Prepares 0.24 for a rolling upgrade to 0.25. This version still uses the fallback strategy to try to deserialize bytes with the new serializer and fall back to the legacy one, in case of failure. However, it first checks the serialized object for a header with a version in order to avoid the slower fallback path in the common case. If the serialized object doesn't have a header, we know it was serialized by 0.24 or earlier.
One note about the structure of the byte header: since Kryo serializes integers with the least significant byte at the lowest address (little endian), we use a header with two bytes in which the first is the version number and the second is a magic byte (0xFF). For a valid registration to conflict with this header, we'd have to register a class with the id 65281 and greater, which is unlikely.
Related issues
closes #5104
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: