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 context timeouts in ServerStreamingAttemptCallable #1155

Merged
merged 2 commits into from Jul 24, 2020

Conversation

noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Jul 21, 2020

Addresses callable overwrite of timeout && streamWaitTimeout mentioned in #1144.

@noahdietz noahdietz requested a review from igorbernstein2 Jul 21, 2020
@google-cla google-cla bot added the cla: yes label Jul 21, 2020
@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Jul 21, 2020

It has been a while since I've touched Java, so please let me know if the tests are wonky.

@codecov
Copy link

@codecov codecov bot commented Jul 21, 2020

Codecov Report

Merging #1155 into master will increase coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1155   +/-   ##
=========================================
  Coverage     78.65%   78.65%           
- Complexity     1170     1172    +2     
=========================================
  Files           204      204           
  Lines          5195     5196    +1     
  Branches        417      418    +1     
=========================================
+ Hits           4086     4087    +1     
  Misses          935      935           
  Partials        174      174           
Impacted Files Coverage Δ Complexity Δ
...le/api/gax/rpc/ServerStreamingAttemptCallable.java 87.82% <33.33%> (+0.10%) 19.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 6ce0c30...d3d7db1. Read the comment docs.

Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

lgtm!

@noahdietz noahdietz requested a review from vam-google Jul 24, 2020
Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Jul 24, 2020

Discussed offline: there might be similar changes to AttemptCallable (for Unary RPCs) necessary, but we are punting on that right now while we investigate how to move forward with changes to timeout/deadline calculation in general. The context timeout is handled slightly differently for Unary RPCs than for Server Streaming. We need to untangle some things.

@noahdietz
Copy link
Contributor Author

@noahdietz noahdietz commented Jul 24, 2020

FYI @chingor13: this shouldn't have any impact on standard client use. This only comes into affect when a caller is explicitly setting a timeout on the context given to a retryable ServerStreaming call via the call(request, context) API. This change honors that caller-provided timeout, rather than silently overwriting it w/totalTimeout as it has been doing.

@noahdietz noahdietz merged commit 461ff84 into googleapis:master Jul 24, 2020
7 of 8 checks passed
@noahdietz noahdietz deleted the streaming-respect-context branch Jul 24, 2020
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