-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update group checksums when combining multiple deltas. #5394
Conversation
The function processing the oracle delta stream combines multiple deltas into one to reduce the number of proposal. However, the group checksums were not being updated, causing some group checksums update to be lost. gRPC guarantees that the ordering of the deltas coming from the stream so overwriting the group checksums is safe. Tested by following the steps in #5368. Fixes #5368
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.
Nice work here!
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)
worker/groups.go, line 950 at r1 (raw file):
delta.Txns = append(delta.Txns, more.Txns...) delta.MaxAssigned = x.Max(delta.MaxAssigned, more.MaxAssigned) delta.GroupChecksums = more.GroupChecksums
Do we need to consider that more.GroupChecksums
might be empty / nil? I think it would be safer to iterate over more.GroupChecksums and set individual keys in delta.GroupChecksums.
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain)
worker/groups.go, line 950 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Do we need to consider that
more.GroupChecksums
might be empty / nil? I think it would be safer to iterate over more.GroupChecksums and set individual keys in delta.GroupChecksums.
Done. I did it just to be safe but I don't think it's necessary. The checksums are obtained by running groupChecksums, which iterates over the whole checksum map.
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.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain)
The function processing the oracle delta stream combines multiple deltas into one to reduce the number of proposal. However, the group checksums were not being updated, causing some group checksums update to be lost. gRPC guarantees that the ordering of the deltas coming from the stream so overwriting the group checksums is safe. Tested by following the steps in #5368. Fixes #5368
The function processing the oracle delta stream combines multiple deltas into one to reduce the number of proposal. However, the group checksums were not being updated, causing some group checksums update to be lost. gRPC guarantees that the ordering of the deltas coming from the stream so overwriting the group checksums is safe. Tested by following the steps in #5368. Fixes #5368
The function processing the oracle delta stream combines multiple deltas into one to reduce the number of proposal. However, the group checksums were not being updated, causing some group checksums update to be lost. gRPC guarantees that the ordering of the deltas coming from the stream so overwriting the group checksums is safe. Tested by following the steps in dgraph-io#5368. Fixes dgraph-io#5368
The function processing the oracle delta stream combines multiple deltas
into one to reduce the number of proposal. However, the group checksums
were not being updated, causing some group checksums update to be lost.
gRPC guarantees that the ordering of the deltas coming from the stream
so overwriting the group checksums is safe.
Tested by following the steps in #5368.
Fixes #5368, DGRAPH-1333
This change is