Skip to content

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

Merged
NamanJain8 merged 5 commits intomasterfrom
naman/fix-subscription
Jan 22, 2021
Merged

fix(subscriptions): fix subscription to use the kv with the max version#7349
NamanJain8 merged 5 commits intomasterfrom
naman/fix-subscription

Conversation

@NamanJain8
Copy link
Copy Markdown
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
Comment thread edgraph/access_ee.go Outdated
Comment thread edgraph/access_ee.go
Comment thread dgraph/cmd/alpha/run.go Outdated
Copy link
Copy Markdown
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
Copy Markdown
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