From ffa36836ab192a6ced60b402b40b9a6b0dc0b434 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Thu, 29 Aug 2024 11:08:31 -0400 Subject: [PATCH 1/5] feat: add toString to futures returned by operations Sometimes an operation can get stuck indefinitely. The underlying reasons can vary significantly: - the underlying attempt rpc can get stuck due to a bug in grpc (ie https://github.com/grpc/grpc-java/pull/11026) - the operation can get stuck in layers above gax: https://github.com/googleapis/java-bigtable/pull/1939 - or it can get stuck in gax itself (dont have a pointer handy) Guava futures provide some observability for ListenableFutures, but in creating the custom ApiFutures in gax, we lose that functionality. This PR sprinkles a few to toString to allow callers to inspect the internal state of the operation. For example with these changes, the toString() of the future returned from bigtableDataClient.mutateRows() changes from TransformFuture@652ce654[status=PENDING, info=[inputFuture=[com.google.api.core.ApiFutureToListenableFuture@522ba524], function=[com.google.api.core.ApiFutures$ApiFunctionToGuavaFunction@29c5ee1d]]] to ListenableFutureToApiFuture{delegate=TransformFuture@7ac9af2a[status=PENDING, info=[inputFuture=[ApiFutureToListenableFuture{apiFuture=CallbackChainRetryingFuture{super=com.google.api.gax.retrying.CallbackChainRetryingFuture@7bb004b8[status=PENDING], latestCompletedAttemptResult=null, attemptResult=null, attemptSettings=TimedAttemptSettings{globalSettings=RetrySettings{totalTimeout=PT10M, initialRetryDelay=PT0.01S, retryDelayMultiplier=2.0, maxRetryDelay=PT1M, maxAttempts=0, jittered=true, initialRpcTimeout=PT1M, rpcTimeoutMultiplier=1.0, maxRpcTimeout=PT1M}, retryDelay=PT0S, rpcTimeout=PT1M, randomizedRetryDelay=PT0S, attemptCount=0, overallAttemptCount=0, firstAttemptStartTimeNanos=635709620001791}}}], function=[com.google.api.core.ApiFutures$ApiFunctionToGuavaFunction@652ce654]]]} This allows us to reason about whats stuck. I'm working another PR that will add a close(timeout) to the Batcher that will use this functionality to identify why batcher.close() timed out --- .../api/core/ApiFutureToListenableFuture.java | 8 +++ .../api/core/ListenableFutureToApiFuture.java | 7 +++ .../core/ApiFutureToListenableFutureTest.java | 54 +++++++++++++++++++ .../core/ListenableFutureToApiFutureTest.java | 18 +++++++ 4 files changed, 87 insertions(+) create mode 100644 api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java diff --git a/api-common-java/src/main/java/com/google/api/core/ApiFutureToListenableFuture.java b/api-common-java/src/main/java/com/google/api/core/ApiFutureToListenableFuture.java index e7c6e9f78a..92f5cf7471 100644 --- a/api-common-java/src/main/java/com/google/api/core/ApiFutureToListenableFuture.java +++ b/api-common-java/src/main/java/com/google/api/core/ApiFutureToListenableFuture.java @@ -29,6 +29,7 @@ */ package com.google.api.core; +import com.google.common.base.MoreObjects; import com.google.common.util.concurrent.ListenableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; @@ -74,4 +75,11 @@ public V get(long l, TimeUnit timeUnit) throws InterruptedException, ExecutionException, TimeoutException { return apiFuture.get(l, timeUnit); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(ApiFutureToListenableFuture.class.getSimpleName()) + .add("apiFuture", apiFuture) + .toString(); + } } diff --git a/api-common-java/src/main/java/com/google/api/core/ListenableFutureToApiFuture.java b/api-common-java/src/main/java/com/google/api/core/ListenableFutureToApiFuture.java index fe77f56d30..5d59da1b71 100644 --- a/api-common-java/src/main/java/com/google/api/core/ListenableFutureToApiFuture.java +++ b/api-common-java/src/main/java/com/google/api/core/ListenableFutureToApiFuture.java @@ -29,6 +29,7 @@ */ package com.google.api.core; +import com.google.common.base.MoreObjects; import com.google.common.util.concurrent.ForwardingListenableFuture.SimpleForwardingListenableFuture; import com.google.common.util.concurrent.ListenableFuture; @@ -39,4 +40,10 @@ public class ListenableFutureToApiFuture extends SimpleForwardingListenableFu public ListenableFutureToApiFuture(ListenableFuture delegate) { super(delegate); } + + public String toString() { + return MoreObjects.toStringHelper(ListenableFutureToApiFuture.class) + .add("delegate", delegate()) + .toString(); + } } diff --git a/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java new file mode 100644 index 0000000000..87bbad4768 --- /dev/null +++ b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java @@ -0,0 +1,54 @@ +/* + * Copyright 2024, Google Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.google.api.core; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ApiFutureToListenableFutureTest { + @Test + public void testThatInnerToStringIsNotLost() { + String customInnerToString = "my-custom-inner-tostring"; + ApiFuture apiFuture = + new AbstractApiFuture() { + @Override + public String toString() { + return customInnerToString; + } + }; + ApiFutureToListenableFuture listenableFuture = + new ApiFutureToListenableFuture<>(apiFuture); + assertThat(listenableFuture.toString()).contains(customInnerToString); + } +} diff --git a/api-common-java/src/test/java/com/google/api/core/ListenableFutureToApiFutureTest.java b/api-common-java/src/test/java/com/google/api/core/ListenableFutureToApiFutureTest.java index 707f7aae0f..3e8d2827d3 100644 --- a/api-common-java/src/test/java/com/google/api/core/ListenableFutureToApiFutureTest.java +++ b/api-common-java/src/test/java/com/google/api/core/ListenableFutureToApiFutureTest.java @@ -30,6 +30,8 @@ package com.google.api.core; import com.google.common.truth.Truth; +import com.google.common.util.concurrent.AbstractFuture; +import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import org.junit.jupiter.api.Test; @@ -42,4 +44,20 @@ void testGet() throws Exception { future.set(3); Truth.assertThat(apiFuture.get()).isEqualTo(3); } + + @Test + void testToStringShowsUnderlyingFutureToString() { + String customInnerFutureDesc = "my-custom-toString-impl"; + ListenableFuture listenableFuture = + new AbstractFuture() { + @Override + public String toString() { + return customInnerFutureDesc; + } + }; + + ListenableFutureToApiFuture apiFuture = + new ListenableFutureToApiFuture<>(listenableFuture); + Truth.assertThat(apiFuture.toString()).contains(customInnerFutureDesc); + } } From fee03b13827383f97e1517f5da0255f643c2dcbd Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Thu, 29 Aug 2024 12:03:06 -0400 Subject: [PATCH 2/5] toString for retrying --- .../google/api/gax/retrying/BasicRetryingFuture.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java b/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java index c1a7ad8898..b21ddaf292 100644 --- a/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java +++ b/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java @@ -35,6 +35,7 @@ import com.google.api.core.ApiFuture; import com.google.api.core.ApiFutures; import com.google.api.gax.tracing.ApiTracer; +import com.google.common.base.MoreObjects; import com.google.common.util.concurrent.AbstractFuture; import com.google.common.util.concurrent.MoreExecutors; import java.util.concurrent.Callable; @@ -265,6 +266,17 @@ private void setAttemptResult(Throwable throwable, ResponseT response, boolean s } } + @Override + public String toString() { + return MoreObjects.toStringHelper(this.getClass()) + .add("super", pendingToString()) + .add("latestCompletedAttemptResult", this.latestCompletedAttemptResult) + .add("attemptResult", this.attemptResult) + .add("attemptSettings", this.attemptSettings) + .toString(); + + } + private class CompletionListener implements Runnable { @Override public void run() { From 5a92d2648c497b979eb1131f3018a512a0ea756d Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Thu, 29 Aug 2024 14:36:10 -0400 Subject: [PATCH 3/5] format --- .../java/com/google/api/gax/retrying/BasicRetryingFuture.java | 1 - 1 file changed, 1 deletion(-) diff --git a/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java b/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java index b21ddaf292..ccf1bfe11c 100644 --- a/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java +++ b/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java @@ -274,7 +274,6 @@ public String toString() { .add("attemptResult", this.attemptResult) .add("attemptSettings", this.attemptSettings) .toString(); - } private class CompletionListener implements Runnable { From 55f57c58452a49eb2abca050f7bab7db44c5b855 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Thu, 29 Aug 2024 17:11:59 -0400 Subject: [PATCH 4/5] use junit5 --- .../com/google/api/core/ApiFutureToListenableFutureTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java index 87bbad4768..bf9dee3a2d 100644 --- a/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java +++ b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java @@ -31,11 +31,8 @@ import static com.google.common.truth.Truth.assertThat; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.jupiter.api.Test; -@RunWith(JUnit4.class) public class ApiFutureToListenableFutureTest { @Test public void testThatInnerToStringIsNotLost() { From 2f05c4232421843d767ef125064fa99ba715c6a5 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Fri, 30 Aug 2024 10:21:08 -0400 Subject: [PATCH 5/5] junit5 codestyle --- .../com/google/api/core/ApiFutureToListenableFutureTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java index bf9dee3a2d..e835c1cd27 100644 --- a/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java +++ b/api-common-java/src/test/java/com/google/api/core/ApiFutureToListenableFutureTest.java @@ -33,9 +33,9 @@ import org.junit.jupiter.api.Test; -public class ApiFutureToListenableFutureTest { +class ApiFutureToListenableFutureTest { @Test - public void testThatInnerToStringIsNotLost() { + void testThatInnerToStringIsNotLost() { String customInnerToString = "my-custom-inner-tostring"; ApiFuture apiFuture = new AbstractApiFuture() {