-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix: backward compatibility with numberofchunks #393
fix: backward compatibility with numberofchunks #393
Conversation
/gcbrun |
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.
Code itself looks good, could you create/modify a test with an entity that has NumberOfChunks
and make sure this loading is done properly?
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.
Overall lgtm, would probably need a few test cases.
return datastore.LoadStruct(b, ps) | ||
// ps_modified has been added to enable backward compatibility by creating | ||
// a new datastore array that replaces legacy "NumberOfChunks" with "ChunkCount". | ||
var ps_modified []datastore.Property |
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.
You can directly change ps
I think. If you need to use a new slice, please use mixedCaps.
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 checked in the history, and "NumberOfChunks" has never been a field of BlobRef. When introduced, it was called ChunkCount. You can remove this change.
internal/pkg/metadb/record/record.go
Outdated
externalBlob, ps, err := timestamps.LoadUUID(ps, externalBlobPropertyName) | ||
// ps_modified has been added to enable backward compatibility by creating | ||
// a new datastore array that replaces legacy "NumberOfChunks" with "ChunkCount". | ||
var ps_modified []datastore.Property |
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.
ditto
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.
Although this fixed the backward compatibility issue within Open Saves, we would still need to change our gRPC client to start passing ChunkCount instead of NumberOfChunks. Since we use a rolling deployment strategy, we need a gRPC client that can handle both fields simultaneously.
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.
Thinking more about this, this code change avoids the internal error we were seeing but won't really provide data consistency since we always read "ChunkCount" from datastore.
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 take this back. I didn't realize the data is already loaded at this point. @zaratsian Are you going to modify the implementation and add a test for this?
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.
That sounds good - I'm working on a few updates based on the feedback, plus the tests and will update the PR shortly.
@zaratsian fyi, please take what you need from here main...yuryu:open-saves:number-count |
Your code suggestions looked great @yuryu, thanks! I've tested several ways as well and all references to NumberOfChunks seems to be handled well. @loranger2k I incorporated your feedback as well, and removed the blobref. I agree that should not be needed because it was introduced after the PR #391. |
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.
looks good.
What type of PR is this?
/kind fix
What this PR does / Why we need it:
This PR maps a legacy field called "NumberOfChunks" to the newly named "ChunkCount" field. This change was implemented against the record and blobref so that all downstream functions are able to reference "ChunkCount" without there being any naming conflict.
Which issue(s) this PR fixes: Backward compatibility issue with Record created with NumberOfChunks #391
#391
Special notes for your reviewer: