-
Notifications
You must be signed in to change notification settings - Fork 119
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
docs: improve timeout and retry sample #2630
Conversation
The timeout and retry sample used 60 seconds for both initial, max, and total timeout. It also used a 1.0 multiplier for the timeout value. This made it impossible to explain how to set an increasing RPC timeout value. It also rendered adding the DEADLINE_EXCEEDED error code as a retryable code superfluous.
@@ -3419,6 +3419,48 @@ public void testRetryOnResourceExhausted() { | |||
} | |||
} | |||
|
|||
@Test | |||
public void testSampleRetrySettings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we carve it into a separate test class similar to what we do for most other samples code? It improves the discoverability of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally only do that for the integration tests that actually execute the sample code. See for example https://github.com/googleapis/java-spanner/blob/main/samples/snippets/src/test/java/com/example/spanner/DmlReturningSampleIT.java. This test does not run the actual sample code, as we don't have any infrastructure in place for running the samples against an in-mem mock server. Instead, it only verifies that not setting DEADLINE_EXCEEDED
as one of the retryable codes will cause the operation to fail with DEADLINE_EXCEEDED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consider adding the new test in a separate class. google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseClientImplTest.java
is already overloaded for testing too many things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the sample test to a separate class (and created an abstract base class for creating mock server tests, so it's easier to set up more of those test classes).
* docs: improve timeout and retry sample The timeout and retry sample used 60 seconds for both initial, max, and total timeout. It also used a 1.0 multiplier for the timeout value. This made it impossible to explain how to set an increasing RPC timeout value. It also rendered adding the DEADLINE_EXCEEDED error code as a retryable code superfluous. * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: update timeout values based on what is possible with other languages * test: move sample test to separate class --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
The timeout and retry sample used 60 seconds for both initial, max, and total timeout. It also used a 1.0 multiplier for the timeout value. This made it impossible to explain how to set an increasing RPC timeout value. It also rendered adding the DEADLINE_EXCEEDED error code as a retryable code superfluous.