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): Fix flaky tests caused by multiple increments of schemaUpdateCounter. #7489

Merged
merged 4 commits into from
Feb 26, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Feb 26, 2021

Fixes GRAPHQL-1050
There were a couple of tests that were behaving flakily because for a single schema update we were incrementing schemaUpdateCounter multiple times, in case the schema update was for a non-Galaxy namespace. This behavior was caused by a bug in schema lazy loading which got added recently with multi-tenancy in PR #7400.

Consider a scenario when we don't have any schema stored in the admin GraphQL server for a non-Galaxy namespace.
Sending a schema POST request at /admin/schema gives a badger subscription update for this schema. Since we don't have any schema in the admin GraphQL server, we update the schemaUpdateCounter but don't update the schema in both the admin and the main GraphQL server, because this schema should be loaded lazily when the main GraphQL server receives a GraphQL request.

Now, what happens is, badger gives another update from the subscription, which contains the same schema with a higher version. Since the admin server didn't store any information about this schema previously, so it is not able to decide whether to reject this update. Resulting in the schemaUpdateCounter being incremented again. Yet, nothing stored in the admin server about the schema.

This results in the schemaUpdateCounter being incremented multiple times for a single schema update request.
This behavior persists until the main GraphQL server doesn't get a request for that namespace resulting in the lazy loading of the schema, which both stores the schema in the admin server and loads it into the main server. Once that happens, updating schema once would only update the schemaUpdateCounter once as well.

So, to fix this bug we introduced a new variable loaded in gqlSchema struct. This flag is set to true whenever a schema is loaded in the main GraphQL server. If there is no schema stored in the admin server or if this flag is false and we get an update from badger subscription, then we just store the gqlSchema on the admin server with this flag set to false, but don't load it into the main GraphQL server. Only if this flag were true, then the schema would be loaded in the main GraphQL server. This mechanism lets the admin server know correctly when to increment the schemaUpdateCounter.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Feb 26, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jatindevdg and @pawanrawal)


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

 ns

it isn't used in the message, should be removed.


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

newSchema.loaded = currentSchema.loaded

lets explicitly set it to true as currentSchema.loaded would be true if it reached here

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 1 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
 ns

it isn't used in the message, should be removed.

done.


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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
newSchema.loaded = currentSchema.loaded

lets explicitly set it to true as currentSchema.loaded would be true if it reached here

done.

@abhimanyusinghgaur abhimanyusinghgaur changed the title fix(GraphQL): fix flaky Test caused by multiple values of schema counter. fix(GraphQL): Fix flaky tests caused by multiple increments of schemaUpdateCounter. Feb 26, 2021
@JatinDev543 JatinDev543 merged commit c1d74c7 into master Feb 26, 2021
@JatinDev543 JatinDev543 deleted the jatin/Fix-lazyLoadCounter-FlakyTest branch February 26, 2021 10:38
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