Make ApiContext timeout relative to the start of each RPC #592

igorbernstein2 · 2018-10-05T06:41:48Z

This partially addresses #561.
Currently the concepts of an RPC timeout and a deadline are a bit mixed up. When a user calls ApiContext.setTimeout(Duration) their duration gets converted into an an absolute deadline. Which is then interpreted as a relative timeout by the retry logic. This results in the issues described in #561.

This PR tries to fix the situation by consistently treating ApiContext.setTimeout(Duration) as a relative RPC timeout. This timeout will be converted into a deadline right before it goes out to gRPC. The only remaining bug from #561 that this doesn't address is when the user sets an absolute deadline on grpc CallOptions and then wraps an ApiContext around it. That will be addressed in a future PR that will introduce absolute deadlines in addition to the relative timeouts introduced here.

This partially addresses googleapis#561. Currently the concepts of an RPC timeout and a deadline are a bit mixed up. When a user calls `ApiContext.setTimeout(Duration)` their duration gets converted into an an absolute deadline. Which is then interpreted as a relative timeout by the retry logic. This results in the issues described in googleapis#561. This PR tries to fix the situation by consistently treating `ApiContext.setTimeout(Duration)` as a relative RPC timeout. This timeout will be converted into a deadline right before it goes out to gRPC. The only remaining bug from googleapis#561 that this doesn't address is when the user sets an absolute deadline on grpc CallOptions and then wraps an ApiContext around it. That will be addressed in a future PR that will introduce absolute deadlines in addition to the relative timeouts introduced here.

igorbernstein2 · 2018-10-05T06:43:39Z

@vam-google @garrettjonesgoogle Please take look when you have a moment

codecov-io · 2018-10-05T06:45:19Z

Codecov Report

Merging #592 into master will increase coverage by 1.05%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##             master     #592      +/-   ##
============================================
+ Coverage     73.96%   75.01%   +1.05%     
- Complexity      906      933      +27     
============================================
  Files           176      176              
  Lines          4067     4074       +7     
  Branches        317      322       +5     
============================================
+ Hits           3008     3056      +48     
+ Misses          907      865      -42     
- Partials        152      153       +1

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/google/api/gax/retrying/RetrySettings.java	`31.25% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../java/com/google/api/gax/grpc/GrpcClientCalls.java	`95.23% <100%> (+1.9%)`	`7 <0> (+3)`	⬆️
...n/java/com/google/api/gax/rpc/AttemptCallable.java	`73.68% <100%> (+1.46%)`	`3 <1> (ø)`	⬇️
...oogle/api/gax/httpjson/HttpJsonDirectCallable.java	`88.23% <83.33%> (+88.23%)`	`4 <0> (+4)`	⬆️
.../java/com/google/api/gax/grpc/GrpcCallContext.java	`84.07% <92.3%> (+2.01%)`	`42 <8> (+4)`	⬆️
...m/google/api/gax/httpjson/HttpJsonCallContext.java	`71.01% <93.33%> (+43.74%)`	`24 <12> (+16)`	⬆️
...va/com/google/api/gax/retrying/RetryAlgorithm.java	`85.71% <0%> (-7.15%)`	`7% <0%> (-1%)`
...api/gax/grpc/InstantiatingGrpcChannelProvider.java	`65.38% <0%> (-3.08%)`	`26% <0%> (ø)`
...om/google/api/gax/httpjson/HttpJsonStatusCode.java	`28.2% <0%> (-2.57%)`	`7% <0%> (-1%)`
...oogle/api/gax/httpjson/ManagedHttpJsonChannel.java	`2% <0%> (+2%)`	`1% <0%> (+1%)`	⬆️
... and 5 more

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 25a19db...7ad7f59. Read the comment docs.

benchmark/src/jmh/java/com/google/api/gax/grpc/ServerStreamingCallableBenchmark.java

                    .setMaxRpcTimeout(Duration.ofHours(1))
                    .build())
            .setIdleTimeout(Duration.ofSeconds(1))
-            .setTimeoutCheckInterval(Duration.ofSeconds(1))


igorbernstein2 · 2018-10-05T19:56:16Z

Added a pile of tests. This PR is complete. Please review and if it looks good, please merge

gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java

+
+  @Nullable
+  @Override
+  public Duration getTimeout() {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java

-      CallOptions newOptions =
-          oldOptions.withDeadlineAfter(rpcTimeout.toMillis(), TimeUnit.MILLISECONDS);
-      GrpcCallContext nextContext = withCallOptions(newOptions);
+  public GrpcCallContext withTimeout(@Nullable Duration rpcTimeout) {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcClientCalls.java

    Preconditions.checkNotNull(callOptions);

+    // Try to convert the timeout into a deadline and use it if it occurs before the actual deadline
+    if (grpcContext.getTimeout() != null) {


gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallContext.java

  private GrpcCallContext(
      Channel channel,
      CallOptions callOptions,
+      @Nullable Duration rpcTimeout,


igorbernstein2 · 2018-10-09T19:43:00Z

@vam-google I've renamed references to rpcTimeout to timeout. PTAL

vam-google

LGTM with few comments. Please address them before pushing (no need to ask for another LGTM)

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java

 @InternalExtensionOnly
 public final class HttpJsonCallContext implements ApiCallContext {
  private final HttpJsonChannel channel;
+  private final Duration rpcTimeout;


gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java

   *
-   * <p>This timeout only applies to a single RPC call; if timeouts are configured, the overall time
-   * taken will be much higher.
+   * <p>This sets the maximum amount of time a single unary RPC attempt can take. If retries are


…#592) This partially addresses googleapis#561. Currently the concepts of an RPC timeout and a deadline are a bit mixed up. When a user calls `ApiContext.setTimeout(Duration)` their duration gets converted into an an absolute deadline. Which is then interpreted as a relative timeout by the retry logic. This results in the issues described in googleapis#561. This PR tries to fix the situation by consistently treating `ApiContext.setTimeout(Duration)` as a relative RPC timeout. This timeout will be converted into a deadline right before it goes out to gRPC. The only remaining bug from googleapis#561 that this doesn't address is when the user sets an absolute deadline on grpc CallOptions and then wraps an ApiContext around it. That will be addressed in a future PR that will introduce absolute deadlines in addition to the relative timeouts introduced here.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 5, 2018

igorbernstein2 added 3 commits October 5, 2018 02:47

more fixes

9af74fb

add some doc about rpc timeouts for server streaming rpcs

195aa9c

fix a compilation error

b2d71d8

igorbernstein2 commented Oct 5, 2018

View reviewed changes

benchmark/src/jmh/java/com/google/api/gax/grpc/ServerStreamingCallableBenchmark.java

.setMaxRpcTimeout(Duration.ofHours(1))

.build())

.setIdleTimeout(Duration.ofSeconds(1))

.setTimeoutCheckInterval(Duration.ofSeconds(1))

This comment was marked as spam.

Sign in to view

igorbernstein2 added 4 commits October 5, 2018 02:57

a bit more doc

e671bd0

more doc

4d6cada

Remove <=0 restriction from timeouts.

3690c5f

fix doc

f4233c5

garrettjonesgoogle requested a review from vam-google October 5, 2018 16:57

igorbernstein2 added 4 commits October 5, 2018 14:35

some tests & fixes

481dab6

more tests

69d4848

more tests

da9c525

more tests

fa2ed7d

vam-google reviewed Oct 8, 2018

View reviewed changes

rename rpcTimeout -> timeout

13db5fd

vam-google approved these changes Oct 9, 2018

View reviewed changes

igorbernstein2 added 2 commits October 9, 2018 16:15

rpcTimeout -> timeout take 2

ffdd77c

address feedback

7ad7f59

igorbernstein2 merged commit ed11321 into googleapis:master Oct 9, 2018

igorbernstein2 deleted the opencensus-1-retry-context branch October 9, 2018 20:31

igorbernstein2 added a commit to igorbernstein2/gax-java that referenced this pull request Oct 9, 2018

Fix typo introduced in googleapis#592.

b669beb

igorbernstein2 added a commit that referenced this pull request Oct 10, 2018

Fix typo introduced in #592. (#601)

12d0c5f

garrettjonesgoogle mentioned this pull request Oct 19, 2018

Release gax-java v1.34.0 #611

Merged

Raibaz pushed a commit to Raibaz/gax-java that referenced this pull request Oct 25, 2018

Fix typo introduced in googleapis#592. (googleapis#601)

b19b2b6

Raibaz pushed a commit to Raibaz/gax-java that referenced this pull request Oct 25, 2018

Fix typo introduced in googleapis#592. (googleapis#601)

b496de1

igorbernstein2 mentioned this pull request Jan 10, 2019

Poor deadline semantics when retries are enabled #561

Closed

igorbernstein2 mentioned this pull request Jul 1, 2020

How does deadline in CallOptions work with Gax Retry settings? #1133

Closed

Make ApiContext timeout relative to the start of each RPC #592

Make ApiContext timeout relative to the start of each RPC #592

Uh oh!

Conversation

igorbernstein2 commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorbernstein2 commented Oct 5, 2018

Uh oh!

codecov-io commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Oct 5, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

igorbernstein2 commented Oct 9, 2018

Uh oh!

vam-google left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

igorbernstein2 commented Oct 5, 2018 •

edited

Loading

codecov-io commented Oct 5, 2018 •

edited

Loading

vam-google left a comment •

edited

Loading