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

Chore(GraphQL): Fix flaky behaviour of TestLargeSchemaUpdate #7522

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Mar 8, 2021

Motivatoin:
TestLargeSchemaUpdate test was failing when race flag is set to true in "Periodic Test All" tests. This PR reduce the size of large schema from 1000 to 250 fields. It also changes the time out of update gql schema request from 50 seconds to 200 seconds.

It took around 80 seconds for the test to run locally with race = true. The timeout has been set to 200 seconds to keep a buffe on slower machinesr.

Testing:

  1. Tested locally with race flag set to true.
  2. Tested without the throttling of indexing code (the test fails in this case as expected)

This change is Reviewable

@vmrajas vmrajas requested a review from pawanrawal as a code owner March 8, 2021 07:12
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 8, 2021
@all-seeing-code
Copy link
Contributor

This reduces the size of the schema and increases the time-out limit. Wondering if it solves the core issue of having a race condition (if at all that's the core issue being identified by --race=true flag). I am afraid that with these changes we might still have the underlying issue affecting the codebase silently. What's your take @abhimanyusinghgaur @vmrajas ?

@vmrajas
Copy link
Contributor Author

vmrajas commented Mar 8, 2021

@anurags92 , There is no race in the test. The reason of failure with --race=true flag is that with the flag set, the test was running slowly causing the updateGQLSchema request to timeout (due to the client timeout set to 50 seconds). As this increases the client timeout to 200 seconds, no such failure is anticipated.

@abhimanyusinghgaur
Copy link
Contributor

Yeah! that makes sense because the more the number of fields in the schema with indexes, the more time schema update will take, and the race flag makes it take even more time.
So, it seems fine to increase the timeout.

Copy link
Contributor

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pawanrawal)

@vmrajas
Copy link
Contributor Author

vmrajas commented Mar 8, 2021

@vmrajas vmrajas merged commit 08f88f8 into master Mar 8, 2021
@vmrajas vmrajas deleted the rajas/fix_largeschematest branch March 8, 2021 08:08
aman-bansal pushed a commit that referenced this pull request Mar 9, 2021
Fix flaky behaviour of TestLargeSchemaUpdate test.
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