From cd31e4f63507fa6c517cf4a16937ae0a61511841 Mon Sep 17 00:00:00 2001 From: noahdietz Date: Mon, 8 Mar 2021 16:45:13 -0800 Subject: [PATCH 1/2] fix: retain user RPC timeout if set --- .../java/com/google/api/gax/rpc/ApiCallContext.java | 10 +++++++--- .../java/com/google/api/gax/rpc/AttemptCallable.java | 3 ++- .../com/google/api/gax/rpc/AttemptCallableTest.java | 3 +++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java b/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java index 1115027d6..e046b1848 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java @@ -63,9 +63,13 @@ public interface ApiCallContext extends RetryingContext { * Returns a new ApiCallContext with the given timeout set. * *

This sets the maximum amount of time a single unary RPC attempt can take. If retries are - * enabled, then this can take much longer. Unlike a deadline, timeouts are relative durations - * that are measure from the beginning of each RPC attempt. Please note that this will limit the - * duration of a server streaming RPC as well. + * enabled, then this can take much longer, as each RPC attempt will have the same constant + * timeout. Unlike a deadline, timeouts are relative durations that are measure from the beginning + * of each RPC attempt. Please note that this will limit the duration of a server streaming RPC as + * well. + * + *

If a method has default {@link com.google.api.gax.retrying.RetrySettings}, the max attempts + * and/or total timeout will still be respected when scheduling each RPC attempt. */ ApiCallContext withTimeout(@Nullable Duration timeout); diff --git a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java index dbd9c995c..949f78e0d 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java +++ b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java @@ -69,8 +69,9 @@ public ResponseT call() { ApiCallContext callContext = originalCallContext; try { + // Only attempt to set the RPC timeout if the caller did not provide their own. Duration rpcTimeout = externalFuture.getAttemptSettings().getRpcTimeout(); - if (!rpcTimeout.isZero()) { + if (!rpcTimeout.isZero() && callContext.getTimeout() == null) { callContext = callContext.withTimeout(rpcTimeout); } diff --git a/gax/src/test/java/com/google/api/gax/rpc/AttemptCallableTest.java b/gax/src/test/java/com/google/api/gax/rpc/AttemptCallableTest.java index cb5133620..3b16b568d 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/AttemptCallableTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/AttemptCallableTest.java @@ -108,6 +108,9 @@ public void testRpcTimeoutIsNotErased() { Duration callerTimeout = Duration.ofMillis(10); ApiCallContext callerCallContext = FakeCallContext.createDefault().withTimeout(callerTimeout); + Duration timeout = Duration.ofMillis(5); + currentAttemptSettings = currentAttemptSettings.toBuilder().setRpcTimeout(timeout).build(); + AttemptCallable callable = new AttemptCallable<>(mockInnerCallable, "fake-request", callerCallContext); callable.setExternalFuture(mockExternalFuture); From c26cac618357be1c99665945dac515f13a6e7d1e Mon Sep 17 00:00:00 2001 From: noahdietz Date: Tue, 9 Mar 2021 08:12:48 -0800 Subject: [PATCH 2/2] address feedback --- gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java | 4 ++-- gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java b/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java index e046b1848..2336be0e7 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java +++ b/gax/src/main/java/com/google/api/gax/rpc/ApiCallContext.java @@ -65,11 +65,11 @@ public interface ApiCallContext extends RetryingContext { *

This sets the maximum amount of time a single unary RPC attempt can take. If retries are * enabled, then this can take much longer, as each RPC attempt will have the same constant * timeout. Unlike a deadline, timeouts are relative durations that are measure from the beginning - * of each RPC attempt. Please note that this will limit the duration of a server streaming RPC as + * of each RPC attempt. Please note that this limits the duration of a server streaming RPC as * well. * *

If a method has default {@link com.google.api.gax.retrying.RetrySettings}, the max attempts - * and/or total timeout will still be respected when scheduling each RPC attempt. + * and/or total timeout is still respected when scheduling each RPC attempt. */ ApiCallContext withTimeout(@Nullable Duration timeout); diff --git a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java index 949f78e0d..e053d5583 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java +++ b/gax/src/main/java/com/google/api/gax/rpc/AttemptCallable.java @@ -69,7 +69,7 @@ public ResponseT call() { ApiCallContext callContext = originalCallContext; try { - // Only attempt to set the RPC timeout if the caller did not provide their own. + // Set the RPC timeout if the caller did not provide their own. Duration rpcTimeout = externalFuture.getAttemptSettings().getRpcTimeout(); if (!rpcTimeout.isZero() && callContext.getTimeout() == null) { callContext = callContext.withTimeout(rpcTimeout);