From 741e40ceb0b7d5e0eceb8a90a52acf57648c0066 Mon Sep 17 00:00:00 2001 From: Diego Alonso Marquez Palacios Date: Tue, 14 Nov 2023 12:00:36 -0500 Subject: [PATCH] fix: improve information on CancellationException for LROs (#2236) * fix: point to documentation on LRO CancelledException * mvn fmt:format * add test for http IT in LRO * format code * use final variable on LRO cancellation * Wrap guava's CancellationException in our own Exception * reset test and basic retry future * use logging to inform of failed LRO * add license and comment to fake log handler * format code * fix license * format souce * decrease LRO timeout logging level to WARNING * fix logging threshold check * use common log handler setup for ITLongRunningOperation * fix license header year * format source * reset lro it * change to unit tests * reset testjar exports * simplify unit test * use instance variables in test --- .../google/api/gax/grpc/ChannelPoolTest.java | 23 +--- gax-java/gax/pom.xml | 1 + .../OperationTimedPollAlgorithm.java | 18 +++ .../OperationTimedPollAlgorithmTest.java | 114 ++++++++++++++++++ .../google/api/gax/util/FakeLogHandler.java | 59 +++++++++ 5 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 gax-java/gax/src/test/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithmTest.java create mode 100644 gax-java/gax/src/test/java/com/google/api/gax/util/FakeLogHandler.java diff --git a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java index 173528d6e2..7131873333 100644 --- a/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java +++ b/gax-java/gax-grpc/src/test/java/com/google/api/gax/grpc/ChannelPoolTest.java @@ -43,6 +43,7 @@ import com.google.api.gax.rpc.StreamController; import com.google.api.gax.rpc.UnaryCallSettings; import com.google.api.gax.rpc.UnaryCallable; +import com.google.api.gax.util.FakeLogHandler; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; @@ -66,9 +67,6 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import java.util.logging.Handler; -import java.util.logging.LogRecord; -import java.util.stream.Collectors; import org.junit.After; import org.junit.Assert; import org.junit.Test; @@ -717,23 +715,4 @@ public void testDoubleRelease() throws Exception { ChannelPool.LOG.removeHandler(logHandler); } } - - private static class FakeLogHandler extends Handler { - List records = new ArrayList<>(); - - @Override - public void publish(LogRecord record) { - records.add(record); - } - - @Override - public void flush() {} - - @Override - public void close() throws SecurityException {} - - List getAllMessages() { - return records.stream().map(LogRecord::getMessage).collect(Collectors.toList()); - } - } } diff --git a/gax-java/gax/pom.xml b/gax-java/gax/pom.xml index d60a739519..a62ab75dbc 100644 --- a/gax-java/gax/pom.xml +++ b/gax-java/gax/pom.xml @@ -76,6 +76,7 @@ com/google/api/gax/core/** com/google/api/gax/rpc/testing/** com/google/api/gax/rpc/mtls/** + com/google/api/gax/util/** diff --git a/gax-java/gax/src/main/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithm.java b/gax-java/gax/src/main/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithm.java index ec7e842e3d..0b2f225dfd 100644 --- a/gax-java/gax/src/main/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithm.java +++ b/gax-java/gax/src/main/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithm.java @@ -35,7 +35,10 @@ import com.google.api.gax.retrying.ExponentialRetryAlgorithm; import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.retrying.TimedAttemptSettings; +import com.google.common.annotations.VisibleForTesting; import java.util.concurrent.CancellationException; +import java.util.logging.Level; +import java.util.logging.Logger; /** * Operation timed polling algorithm, which uses exponential backoff factor for determining when the @@ -43,6 +46,14 @@ * algorithm cancels polling. */ public class OperationTimedPollAlgorithm extends ExponentialRetryAlgorithm { + + @VisibleForTesting + static final Logger LOGGER = Logger.getLogger(OperationTimedPollAlgorithm.class.getName()); + + @VisibleForTesting + static final String LRO_TROUBLESHOOTING_LINK = + "https://github.com/googleapis/google-cloud-java#lro-timeouts"; + /** * Creates the polling algorithm, using the default {@code NanoClock} for time computations. * @@ -77,6 +88,13 @@ public boolean shouldRetry(TimedAttemptSettings nextAttemptSettings) if (super.shouldRetry(nextAttemptSettings)) { return true; } + if (LOGGER.isLoggable(Level.WARNING)) { + LOGGER.log( + Level.WARNING, + "The task has been cancelled. Please refer to " + + LRO_TROUBLESHOOTING_LINK + + " for more information"); + } throw new CancellationException(); } diff --git a/gax-java/gax/src/test/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithmTest.java b/gax-java/gax/src/test/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithmTest.java new file mode 100644 index 0000000000..583afcc819 --- /dev/null +++ b/gax-java/gax/src/test/java/com/google/api/gax/longrunning/OperationTimedPollAlgorithmTest.java @@ -0,0 +1,114 @@ +/* + * Copyright 2023 Google LLC + * + * 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 LLC 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.gax.longrunning; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.google.api.gax.core.FakeApiClock; +import com.google.api.gax.retrying.RetrySettings; +import com.google.api.gax.retrying.TimedAttemptSettings; +import com.google.api.gax.util.FakeLogHandler; +import java.util.concurrent.CancellationException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.threeten.bp.Duration; + +public class OperationTimedPollAlgorithmTest { + + private static final RetrySettings FAST_RETRY_SETTINGS = + RetrySettings.newBuilder() + .setInitialRetryDelay(Duration.ofMillis(1L)) + .setRetryDelayMultiplier(1) + .setMaxRetryDelay(Duration.ofMillis(1L)) + .setInitialRpcTimeout(Duration.ofMillis(1L)) + .setMaxAttempts(0) + .setJittered(false) + .setRpcTimeoutMultiplier(1) + .setMaxRpcTimeout(Duration.ofMillis(1L)) + .setTotalTimeout(Duration.ofMillis(5L)) + .build(); + private TimedAttemptSettings timedAttemptSettings; + private FakeApiClock clock; + + private FakeLogHandler logHandler; + + @Before + public void setUp() { + logHandler = new FakeLogHandler(); + OperationTimedPollAlgorithm.LOGGER.addHandler(logHandler); + clock = new FakeApiClock(System.nanoTime()); + timedAttemptSettings = + TimedAttemptSettings.newBuilder() + .setGlobalSettings(FAST_RETRY_SETTINGS) + .setRetryDelay(Duration.ofMillis(1l)) + .setRpcTimeout(Duration.ofMillis(1l)) + .setRandomizedRetryDelay(Duration.ofMillis(1l)) + .setAttemptCount(0) + .setFirstAttemptStartTimeNanos(clock.nanoTime()) + .build(); + } + + @After + public void tearDown() { + OperationTimedPollAlgorithm.LOGGER.removeHandler(logHandler); + // redundant null assignment for readability - a new log handler will be used + logHandler = null; + } + + @Test + public void testAlgorithmThatShouldRetry_doesNotLogTimeoutHelpMessage() { + OperationTimedPollAlgorithm algorithm = + OperationTimedPollAlgorithm.create(FAST_RETRY_SETTINGS, clock); + try { + algorithm.shouldRetry(timedAttemptSettings); + } catch (CancellationException ex) { + fail("Unexpected unsuccessful shouldRetry()"); + } + assertTrue( + logHandler.getAllMessages().stream() + .noneMatch( + entry -> entry.contains(OperationTimedPollAlgorithm.LRO_TROUBLESHOOTING_LINK))); + } + + @Test + public void testAlgorithmThatShouldNotRetry_logsTimeoutHelpMessage() { + OperationTimedPollAlgorithm algorithm = + OperationTimedPollAlgorithm.create(FAST_RETRY_SETTINGS, clock); + clock.incrementNanoTime(1 * 1000 * 1000 * 1000); // force rpc timeout + assertThrows(CancellationException.class, () -> algorithm.shouldRetry(timedAttemptSettings)); + assertTrue( + logHandler.getAllMessages().stream() + .anyMatch( + entry -> entry.contains(OperationTimedPollAlgorithm.LRO_TROUBLESHOOTING_LINK))); + } +} diff --git a/gax-java/gax/src/test/java/com/google/api/gax/util/FakeLogHandler.java b/gax-java/gax/src/test/java/com/google/api/gax/util/FakeLogHandler.java new file mode 100644 index 0000000000..f6dbef78ea --- /dev/null +++ b/gax-java/gax/src/test/java/com/google/api/gax/util/FakeLogHandler.java @@ -0,0 +1,59 @@ +/* + * Copyright 2023 Google LLC + * + * 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 LLC 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.gax.util; + +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.stream.Collectors; + +/* + * Convenience class that stores the log entries produced by any logger + * It can then be inspected - its entries are a list log records + */ +public class FakeLogHandler extends Handler { + List records = new ArrayList<>(); + + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() {} + + @Override + public void close() throws SecurityException {} + + public List getAllMessages() { + return records.stream().map(LogRecord::getMessage).collect(Collectors.toList()); + } +}