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: retain user RPC timeout if set via withTimeout #1324

Merged
merged 2 commits into from Mar 10, 2021

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Mar 9, 2021

If a Callable has RetrySettings and the calculated RPC Timeout for an attempt is less than the constant, RPC timeout provided by the user via ApiCallContext.withTimeout, the attempt settings will win. This is not caller friendly.

This PR changes AttemptCallable, used for Unary RPCs, only attempts to set the ApiCallContext.withTimeout when it is not already set by the caller.

This was fixed for ServerStreaming RPCs in #1155

Addresses commentary in #1144.

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

@codecov codecov bot commented Mar 9, 2021

Codecov Report

Merging #1324 (c26cac6) into master (27d92c6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1324   +/-   ##
=========================================
  Coverage     79.65%   79.65%           
- Complexity     1248     1249    +1     
=========================================
  Files           209      209           
  Lines          5461     5461           
  Branches        464      464           
=========================================
  Hits           4350     4350           
  Misses          928      928           
  Partials        183      183           
Impacted Files Coverage Δ Complexity Δ
...n/java/com/google/api/gax/rpc/AttemptCallable.java 82.60% <100.00%> (ø) 5.00 <0.00> (+1.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 27d92c6...c26cac6. Read the comment docs.

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Mar 10, 2021

@elharo do you want to take another pass or am I OK to merge?

Copy link
Contributor

@elharo elharo left a comment

You don't need to wait for me. If it's approved, you can merge

@noahdietz noahdietz merged commit 3fe1db9 into googleapis:master Mar 10, 2021
10 checks passed
@noahdietz noahdietz deleted the retain-with-timeout branch Mar 10, 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.

None yet

3 participants