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

Correctness Fix: Block before proposing mutations and improve conflict key generation #3565

Merged
merged 8 commits into from
Jun 27, 2019

Conversation

manishrjain
Copy link
Contributor

@manishrjain manishrjain commented Jun 16, 2019

This PR fixes two things:

  • Before proposing mutations, Dgraph didn't block to ensure that it has seen all the committed transaction updates. So, the index calculation had side-effects as shown by count index tests, where a previous update hasn't been registered yet and a new one is made, which caused incorrect count to be generated, hence causing the count index to be wrong.

This PR fixes that by ensuring that Dgraph waits for the Alpha involved to have reached StartTs. Only then would it propose a new update. This ensures that all the updates are in correct order.

  • Conflict Keys: Previously, Dgraph was generating conflict keys based on (S, P) for all list values, and (S, P, O) for all uids. This PR changes that by using (S, P) for singular predicates, without considering whether they are uid or value; and using (S, P, O) for list uids/values. This PR also adds conflict keys for indices, treating them as list of uids.

  • Based on tests by @danielmai , this PR does not have any impact on the live loader throughput.


This change is Reviewable

@manishrjain manishrjain requested review from martinmr and a team as code owners June 16, 2019 19:15
@manishrjain manishrjain requested a review from gitlw June 16, 2019 19:23
Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

The tests are failing, I've triggered another run of the tests to rule out transient failures.
Other than that, the PR LGTM.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @gitlw and @martinmr)

Copy link

@gitlw gitlw 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 4 files reviewed, all discussions resolved (waiting on @gitlw and @martinmr)

Copy link
Contributor

@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.

:lgtm: Just one minor comment.

Reviewed 4 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain)


worker/groups.go, line 203 at r2 (raw file):

	// Propose schema mutation.
	var m pb.Mutations
	// schema for a reserved predicate is not changed once set.

minor: maybe move or remove this comment. It seems out of context now.

@gitlw gitlw merged commit 693e7db into master Jun 27, 2019
@gitlw gitlw deleted the mrjn/conflict-keys branch June 27, 2019 23:02
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
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.

3 participants