-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: Add schema version id to commit queries #1061
feat: Add schema version id to commit queries #1061
Conversation
6b75761
to
79ad6f0
Compare
Spotted whilst looking for a type, this variable is no longer used and can be removed
Will be added to shortly
Codecov Report
@@ Coverage Diff @@
## develop #1061 +/- ##
===========================================
+ Coverage 68.19% 68.21% +0.01%
===========================================
Files 173 173
Lines 16330 16338 +8
===========================================
+ Hits 11137 11145 +8
Misses 4261 4261
Partials 932 932
|
79ad6f0
to
3fa9e4f
Compare
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. Just one suggestion that you can apply or not :)
SchemaVersionID string | ||
Priority uint64 | ||
Data []byte | ||
DocKey []byte | ||
}{delta.SchemaVersionID, delta.Priority, delta.Data, delta.DocKey}) |
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.
suggestion: Since the struct represents the LWWRegDelta
struct, you could probably just use
LWWRegDelta{delta.SchemaVersionID, delta.Priority, delta.Data, delta.DocKey}
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.
Discussed a bit in the standup - this comment prompted the creation of #1065. In the short term I'll leave this as is, as it is not new and can be sorted out better if done in a dedicated yet broader ticket.
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
* Remove deadcode Spotted whilst looking for a type, this variable is no longer used and can be removed * Break up long line Will be added to shortly * Persist schemaVersionId on field commits * Add SchemaVersionId to commit query
* Remove deadcode Spotted whilst looking for a type, this variable is no longer used and can be removed * Break up long line Will be added to shortly * Persist schemaVersionId on field commits * Add SchemaVersionId to commit query
Relevant issue(s)
Resolves #1006
Description
Adds schema version id to commit queries.
Also persists schema version id on field commit blocks as discussed earlier - this is because it is quite difficult to find the composite commit from the field commit, as well as being a little strange ideologically. In the future we may break this property out to a new index/key to save space/duplication, but not now.