fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings #1603

lqiu96 · 2023-04-03T22:23:50Z

We should be able to cancel the runnable/ close the call exactly when the RPC timeout has elapsed.

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

blakeli0 · 2023-04-07T18:07:32Z

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

@@ -88,6 +93,7 @@
  private final ApiMethodDescriptor<RequestT, ResponseT> methodDescriptor;
  private final HttpTransport httpTransport;
  private final Executor executor;
+  private final ScheduledExecutorService deadlineCancellationExecutor;


Can we try to reuse the executor above? I know it's not a ScheduledExecutorService at this point, but the what's being passed in is from here, which is indeed a ScheduledExecutorService, so we should be able to change the type and reuse it. I did a quick search and I don't think most of the change would be breaking, there is one public builder method that could be considered breaking.
The reason we may want to reuse the existing executor is that we can leverage the existing client lifecycles for shutting down the executor, otherwise we need to explicitly shut down this new deadlineCancellationExecutor somewhere to prevent from resource leaking, which is missing from this PR currently. Reusing the existing executor may also have other issues like the ThreadPool is not large enough so new requests may have to wait for new thread. The bottom line is that we need to be careful with anything related to executors, so we have enough resource for large number of concurrent requests and we don't leak resource accidentally.

Gotcha, makes sense. I'll take another look into how grpc handles the multiple executors. I'd imagine they would also possibly run into this issue.

blakeli0 · 2023-04-07T18:17:21Z

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

+  // there is a timeout exception from this RPC call (DEADLINE_EXCEEDED)
+  private void closeAndNotifyListeners() {
+    synchronized (lock) {
+      close(


This comment in close method makes me worried that we may still have to wait for the server to fully send the response.

Yeah the cancellation in the runnable doesn't seem like it's actually cancel's the runnable. I'll look into the possibility of being able to actually disconnect the connection instead of waiting for the socket timeout.

blakeli0 · 2023-05-05T15:49:55Z

gax-java/gax/src/main/java/com/google/api/gax/retrying/ExponentialRetryAlgorithm.java

+  // will occur immediately). For any other value, the deadlineScheduler will
+  // terminate in the future (even if the timeout is small).
+  @InternalApi
+  protected boolean shouldRPCTerminate(long timeLeftMs) {


Do we still need this new method now that we are using ms?

Yeah, this is here due the the very subtle difference between unary/streaming vs LRO retry logic. Polling from LRO's should retry even if timeLeftMs == 0 (as it would be the final polling attempt), but unary/streaming callables should not (and it would signify an rpc timeout attempt of 0)

blakeli0 · 2023-05-05T15:58:54Z

...case/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITLongRunningOperation.java

+  @Test
+  public void testGRPC_LROSuccessfulResponse_NoDeadlineExceeded()
+      throws IOException, ExecutionException, InterruptedException {
+    EchoStubSettings.Builder grpcEchoSettingsBuilder = EchoStubSettings.newBuilder();


These setups are 40 lines long and made the tests hard to read, can we extract the common set ups to a private method or setup method or Util class?

Ah yep, will do.

blakeli0 · 2023-05-05T16:00:42Z

...case/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITLongRunningOperation.java

+public class ITLongRunningOperation {
+
+  @Test
+  public void testGRPC_LROSuccessfulResponse_NoDeadlineExceeded()


Does the test name still reflect what we are testing? Or maybe we need to make it clearer?
I see NoDeadlineExceeded which assumes not retry, but I see an assertion in the end that asserts attemptCount to be at least 2.

Ah yep, I can update the test names to better clarity. Some tests cases are testing that the deadline exceeded and that it throws a DEADLINE_EXCEEDED exception and it's very easy to confuse.

blakeli0 · 2023-05-05T16:07:06Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITUnaryDeadline.java

+            // Explicitly set retries as disabled (maxAttempts == 1)
+            .setMaxAttempts(1)
+            .build();
+    EchoStubSettings.Builder grpcEchoSettingsBuilder = EchoStubSettings.newBuilder();


Same thing for the setups in this class, it would be great if we can simplify them. Maybe we can expose a method in TestClientInitializer that takes RetrySettings and RetryableCodes as parameters.

Yep will do! That's a good idea.

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITUnaryDeadline.java

blakeli0 · 2023-05-05T16:29:58Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITUnaryDeadline.java

+ * <p>Each test attempts to get the number of attempts done in each call. The attemptCount is
+ * incremented by 1 as the first attempt is zero indexed.
+ */
+public class ITUnaryDeadline {


Not part of this PR. It would be great if we can programmatically start/stop the showcase server so we can test things like connect/socket timeouts.

I can raise in the showcase repo and see what the others think (or see if that's possible / easily done).

I meant more like doing it in our own repo, but yeah asking others if it has been done before is a good idea.

blakeli0 · 2023-05-05T17:05:28Z

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

-    if (newDeadline != null) {
-      builder.setDeadline(newDeadline);
+    if (inputOptions.getTimeout() != null) {
+      Duration newTimeout = java.time.Duration.ofNanos(inputOptions.getTimeout().getNano());


We can use ms instead of nanos now?

sonarcloud · 2023-05-08T19:04:49Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

82.4% Coverage
0.0% Duplication

sonarcloud · 2023-05-08T19:08:15Z

[java_showcase_integration_tests] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

72.9% Coverage
0.0% Duplication

sonarcloud · 2023-05-08T19:12:55Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

83.5% Coverage
0.0% Duplication

lqiu96 added 2 commits March 31, 2023 14:43

chore: Add retry test

dca83d7

chore: Check the timeout for unary callables

45c0765

product-auto-label bot added the size: m Pull request size is medium. label Apr 3, 2023

lqiu96 changed the title ~~Main showcase retries~~ fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings Apr 3, 2023

lqiu96 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 3, 2023

lqiu96 added 7 commits April 3, 2023 23:31

chore: Fix tests

7862762

chore: Address code smell

7f5622f

chore: Add tests for DEADLINE_EXCEEDED

94b5017

chore: Add tests for Server-side streaming

608d44b

chore: Start the timeout after http request invocation

0c1782f

chore: Fix format issues

f75f67e

chore: Remove Instant calculation with System clock

7c4c841

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 5, 2023

lqiu96 added 9 commits April 5, 2023 15:23

Merge branch 'main' into main-showcase_retries

3668093

chore: Add ITRetry test cases

d440b8e

chore: Fix the Retry showcase test

bbf7f4f

chore: Fix the Retry showcase test

bee8c0e

chore: Convert duration to nanos

73882a7

chore: Set the readTimeout min to 20s

b0d83b9

chore: Add sucessful retry tests cases

fc5cf13

chore: Add comments to timeout

2f1bc8a

chore: Update the connect timeout

ead0eb9

lqiu96 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 6, 2023

lqiu96 requested a review from blakeli0 April 6, 2023 20:28

lqiu96 marked this pull request as ready for review April 6, 2023 20:28

lqiu96 requested a review from a team as a code owner April 6, 2023 20:28

blakeli0 reviewed Apr 7, 2023

View reviewed changes

lqiu96 added 2 commits April 7, 2023 17:21

chore: Refactor timeout logic

ca0770b

chore: Fix format issues

21ed291

lqiu96 added 13 commits May 1, 2023 18:01

chore: Address PR comments

ed6b53c

chore: Update java.time.Duration import

2095745

chore: Update values for LRO showcase test

a8e3153

chore: Update values for LRO showcase test

760f5fe

chore: Fix format issues

d890dee

chore: Update variable name

3086821

chore: Convert shouldRetry logic to use milliseconds

084b592

chore: Remove jitter from tests

145d084

chore: Fix showcase test

e9c1645

chore: Log the attempt callable timeout

600a2dc

chore: Update LRO test case

5d81e85

chore: Add logging

c429d96

chore: Use millis for timeout

5527700

blakeli0 reviewed May 5, 2023

View reviewed changes

lqiu96 added 6 commits May 5, 2023 14:26

chore: Address PR comments

13881bb

chore: Update to use TestClientInitializer class

c052a87

Merge branch 'main' into main-showcase_retries

069acc6

chore: Fix client initialize method names

6cc15fb

chore: Add back public method

e099ac1

chore: Add back public methods

0a3cadf

blakeli0 approved these changes May 8, 2023

View reviewed changes

lqiu96 merged commit d2fe520 into main May 8, 2023
18 of 19 checks passed

lqiu96 deleted the main-showcase_retries branch May 8, 2023 20:18

release-please bot mentioned this pull request May 8, 2023

chore(main): release 2.19.0 #1662

Merged

meltsufin mentioned this pull request Jan 12, 2024

google-cloud-compute: ImagesClient does not terminate, and hangs instead googleapis/google-cloud-java#10220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings #1603

fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings #1603

lqiu96 commented Apr 3, 2023 •

edited

blakeli0 Apr 7, 2023

lqiu96 Apr 10, 2023

blakeli0 Apr 7, 2023

lqiu96 Apr 10, 2023

blakeli0 May 5, 2023

lqiu96 May 5, 2023

blakeli0 May 5, 2023 •

edited

lqiu96 May 5, 2023

blakeli0 May 5, 2023

lqiu96 May 5, 2023

blakeli0 May 5, 2023

lqiu96 May 5, 2023

blakeli0 May 5, 2023

lqiu96 May 5, 2023 •

edited

blakeli0 May 5, 2023

blakeli0 May 5, 2023

lqiu96 May 5, 2023

sonarcloud bot commented May 8, 2023

sonarcloud bot commented May 8, 2023

sonarcloud bot commented May 8, 2023

fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings #1603

fix: Unary Callables Deadline values respect the TotalTimeout in RetrySettings #1603

Conversation

lqiu96 commented Apr 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 May 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 8, 2023

sonarcloud bot commented May 8, 2023

sonarcloud bot commented May 8, 2023

lqiu96 commented Apr 3, 2023 •

edited

blakeli0 May 5, 2023 •

edited

lqiu96 May 5, 2023 •

edited