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

feat: wrap non-retryable RPCs in retry machinery #1328

Merged
merged 9 commits into from Apr 5, 2021

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Mar 12, 2021

RPCs that are configured at the client-level to be non-retryable still get wrapped in the retry machinery to ensure features like #1238 that depend on call-time context options still work for non-retryable methods.

In order to ensure that the proper timeout is used by default, the call settings are overridden with setSimpleTimeoutNoRetries.

There are already tests that verify the proper timeout is used for non-retryable RPCs, for example:

public void testNonRetryUnarySettings() {

Fixes #1327

@noahdietz noahdietz requested review from as code owners Mar 12, 2021
@google-cla google-cla bot added the cla: yes label Mar 12, 2021
@codecov
Copy link

@codecov codecov bot commented Mar 12, 2021

Codecov Report

Merging #1328 (666fc7a) into master (751ccf3) will increase coverage by 0.82%.
The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1328      +/-   ##
============================================
+ Coverage     80.56%   81.38%   +0.82%     
- Complexity     1332     1340       +8     
============================================
  Files           210      210              
  Lines          5686     5690       +4     
  Branches        519      521       +2     
============================================
+ Hits           4581     4631      +50     
+ Misses          894      850      -44     
+ Partials        211      209       -2     
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/google/api/gax/rpc/Callables.java 87.27% <93.33%> (+18.64%) 13.00 <3.00> (+1.00)
...le/api/gax/rpc/ServerStreamingAttemptCallable.java 89.56% <0.00%> (+1.73%) 21.00% <0.00%> (+2.00%)
...a/com/google/api/gax/rpc/BatchingCallSettings.java 100.00% <0.00%> (+5.00%) 8.00% <0.00%> (ø%)
.../google/api/gax/batching/NonBlockingSemaphore.java 86.84% <0.00%> (+5.26%) 13.00% <0.00%> (+1.00%)
...java/com/google/api/gax/rpc/PagedCallSettings.java 100.00% <0.00%> (+10.00%) 4.00% <0.00%> (ø%)
...i/gax/retrying/SimpleStreamResumptionStrategy.java 28.57% <0.00%> (+14.28%) 2.00% <0.00%> (+1.00%)
...oogle/api/gax/rpc/ServerStreamingCallSettings.java 93.44% <0.00%> (+22.95%) 8.00% <0.00%> (+1.00%)
...e/api/gax/rpc/RetryingServerStreamingCallable.java 70.00% <0.00%> (+70.00%) 2.00% <0.00%> (+2.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 751ccf3...666fc7a. Read the comment docs.

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 16, 2021

going to add a CallablesTest.java because there really should be unit tests for these functions

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 16, 2021

going to add a CallablesTest.java because there really should be unit tests for these functions

Never mind, this code is actually covered by the TimeoutTest file, that's just not in the same package and thus doesn't cover.

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 16, 2021

@igorbernstein2 what are the implications of this change on ServerStreaming callables? Would making all callables wrapped in in the Retry machinery be bad for ServerStreaming?

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 17, 2021

I spoke with Igor offline and he wants to look into how this might affect tracing data for non-retryable RPCs (b.c now the RPC goes through the retry machinery for the single attempt made). He will follow up here with any findings.

He did say this shouldn't be a problem for server streaming RPCs, though.

@noahdietz noahdietz requested a review from igorbernstein2 Mar 18, 2021
@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 24, 2021

I've added more test classes to verify context & tracer behavior with non-retryable RPCs (and acquire proper code coverage).

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 29, 2021

@igorbernstein2 have you had a chance to verify things locally or see if the tests I added were sufficient WRT Tracing? I'd like to merge this so it can ship with the next release. Thank you!

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Apr 2, 2021

I'm going to submit this PR on Monday.

@noahdietz noahdietz merged commit 51c40ab into googleapis:master Apr 5, 2021
9 checks passed
@noahdietz noahdietz deleted the wrap-callables branch Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants