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: respect total request timeout for Query retries #806

Merged
merged 3 commits into from Oct 29, 2021

Conversation

schmidt-sebastian
Copy link
Contributor

This PR addresses two issue in the Query retry logic:

  • We should only retry queries that fail intermittently if at least one document result was received. Otherwise, we risk retrying in a loop without backoff.
  • We should not retry longer than the total request timeout that the customer can optionally specify.

@schmidt-sebastian schmidt-sebastian requested a review from a team as a code owner October 28, 2021 21:20
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Oct 28, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2021
}
}
Query.this
.startAfter(cursor)
Copy link
Contributor

Choose a reason for hiding this comment

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

On line 1073 the startAfter() method annotates its argument with @Nonnull; however, it appears that cursor could be null since the null check has been removed from this code here. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. shouldRetry() checks that it won't be null. Maybe this could be clearer by modifying shouldRetry() to take the cursor as an argument? Can you think of a way to make it clearer that cursor is guaranteed to not be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rearranged the code a bit and used "cursor" as an argument. Let me know if this is what you had in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is what I had in mind. Thanks.

}
}
Query.this
.startAfter(cursor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is what I had in mind. Thanks.

@schmidt-sebastian schmidt-sebastian merged commit feb1921 into main Oct 29, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/retries branch October 29, 2021 16:11
gcf-merge-on-green bot pushed a commit that referenced this pull request Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants