Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: retain context timeouts in ServerStreamingAttemptCallable #1155

Merged
merged 2 commits into from
Jul 24, 2020

Conversation

noahdietz
Copy link
Contributor

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

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 21, 2020
@noahdietz
Copy link
Contributor Author

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

@codecov
Copy link

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
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM

@noahdietz
Copy link
Contributor Author

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

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
@noahdietz noahdietz deleted the streaming-respect-context branch July 24, 2020 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants