From ffa36836ab192a6ced60b402b40b9a6b0dc0b434 Mon Sep 17 00:00:00 2001 From: Igor Berntein Date: Thu, 29 Aug 2024 11:08:31 -0400 Subject: [PATCH] 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); + } }