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

Update group checksums when combining multiple deltas. #5394

Merged
merged 2 commits into from
May 8, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented May 7, 2020

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 Reviewable

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
@martinmr martinmr requested review from manishrjain and a team as code owners May 7, 2020 22:45
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Got a comment. Do address.

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.

Copy link
Contributor Author

@martinmr martinmr left a 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.

@martinmr martinmr requested a review from manishrjain May 7, 2020 22:55
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain)

@martinmr martinmr merged commit 041d5e6 into master May 8, 2020
@martinmr martinmr deleted the martinmr/fix-checksums branch May 8, 2020 16:35
martinmr added a commit that referenced this pull request May 8, 2020
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
martinmr added a commit that referenced this pull request May 8, 2020
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
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Group checksum mismatch and pending mutation blocks read-only queries indefinitely
2 participants