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(GraphQL):This PR will fix the graphl tests that were failing because of recent change in dgraph. #7329

Merged
merged 7 commits into from
Jan 20, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Jan 19, 2021

we have changed in Dgraph the way rollups are done in this PR. #7253
As a result of it, we were having below unexpected behavior in graphql while updating the schema.
issue:
suppose we send below schema requests,

1.schema1
2.schema2

Badger is supposed to return the following updates to graphql

1.schema1
1.schema1
1.schema1
2.schema2
2.schema2
2.schema2

If we get a schema update that is same as our previous schema then we are already ignoring that
in graphql.For example, below multiple updates for schema1,schema2 from badger gets ignored

1.schema1  //accepted,new update
2.schema1  //discarded,same as previous
3.schema1  //discarded,same as previous
4.schema1  //discared,same as previous
5.schema2  //accepted,new update 
6.schema2  //discarded,same as previous
7.schema2  //discarded,same as previous

But because of the recent change we are now sometimes getting older updates in random order like below.

1.schema1  //version1
2.schema2  //version2
3.schema1  //version1

so in this case graphql was assuming that we got an new schema which actually was old schema.
And due to that multiple errors were broken.

Solution
The solution is simple.Every update from the badger also have version associated to it.
And that version number is strictly increasing. Now, if we get an schema with version <= current schema version
then we don't update the graphql schema.And that solve the issue


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jan 19, 2021
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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jatindevdg)


graphql/admin/admin.go, line 540 at r1 (raw file):

		if newSchema.Version <= server.schema.Version || newSchema.Schema == server.schema.Schema {
			glog.Infof("Skipping GraphQL schema update because either we got same schema or schema with older" +
				" version than version of current schema ")

Skipping GraphQL schema update, new badger key version is %d, the old version was %d.

Copy link
Contributor Author

@JatinDev543 JatinDev543 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 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


graphql/admin/admin.go, line 540 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Skipping GraphQL schema update, new badger key version is %d, the old version was %d.

done.

@pawanrawal pawanrawal merged commit cc506cc into master Jan 20, 2021
@pawanrawal pawanrawal deleted the jatin/GRAPHQL-Fix-Tests branch January 20, 2021 13:49
JatinDev543 added a commit that referenced this pull request Jan 21, 2021
…extra schema updates (#7329)

Don't update GraphQL schema if an older version of schema key is received.

(cherry picked from commit cc506cc)
JatinDev543 added a commit that referenced this pull request Jan 21, 2021
…extra schema updates (#7329) (#7348)

Don't update GraphQL schema if an older version of schema key is received.

(cherry picked from commit cc506cc)
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.

2 participants