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

fix(subscriptions): fix subscription to use the kv with the max version #7349

Merged
merged 5 commits into from
Jan 22, 2021

Conversation

NamanJain8
Copy link
Contributor

@NamanJain8 NamanJain8 commented Jan 21, 2021

Issue:
The last KV from the list received from subscription need not be the latest update. This is because of the KVs written at the top due to value log rewrites or due to rollups.
We were using the last KV from the updates received via subscription.

Fix:
Iterate over the KVs to get the KV with the latest version.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jan 21, 2021
edgraph/access_ee.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @NamanJain8, and @vvbalaji-dgraph)


x/x.go, line 1219 at r3 (raw file):

	// Iterate over kvs to get the KV with the latest version. It is not necessary that the last
	// KV contain the latest value.
	var maxKv badgerpb.KV

Why not define it like

var maxKv *badgerpb.KV

and then below you can do
maxKv = kv

...
return maxKv

Copy link
Contributor Author

@NamanJain8 NamanJain8 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: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


dgraph/cmd/alpha/run.go, line 815 at r1 (raw file):

Previously, NamanJain8 (Naman Jain) wrote…

I don't think there will be any benefit in comparing the key, unless we suspect that badger might have sent the key which does not match the subscribed prefixes.

Done.


x/x.go, line 1219 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why not define it like

var maxKv *badgerpb.KV

and then below you can do
maxKv = kv

...
return maxKv

Thanks. Done.

@NamanJain8 NamanJain8 merged commit 4a2bc36 into master Jan 22, 2021
@NamanJain8 NamanJain8 deleted the naman/fix-subscription branch January 22, 2021 13:55
NamanJain8 added a commit that referenced this pull request Jan 22, 2021
…on (#7349)

Issue:
The last KV from the list received from subscription need not be the latest update. This is because of the KVs written at the top due to value log rewrites or due to rollups.
We were using the last KV from the updates received via subscription.

Fix:
Iterate over the KVs to get the KV with the latest version.

(cherry picked from commit 4a2bc36)
aman-bansal pushed a commit that referenced this pull request Jan 22, 2021
…on (#7349) (#7355)

Issue:
The last KV from the list received from subscription need not be the latest update. This is because of the KVs written at the top due to value log rewrites or due to rollups.
We were using the last KV from the updates received via subscription.

Fix:
Iterate over the KVs to get the KV with the latest version.

(cherry picked from commit 4a2bc36)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

3 participants