From 675e6fbb8beb520c18c43155e02ab6e7b8e27a3a Mon Sep 17 00:00:00 2001 From: vam-google Date: Thu, 4 Jun 2020 16:24:46 -0700 Subject: [PATCH 1/2] fix: Make watchdog and `@BetaApi` and watchdog provider implementations `@InternalApi` This is to address https://github.com/googleapis/gax-java/issues/829. Also add grpclb lib to bazel dependencies, as its absence was failing some of the tests well executing from bazel. --- gax-grpc/BUILD.bazel | 1 + .../google/api/gax/rpc/FixedWatchdogProvider.java | 4 +++- .../api/gax/rpc/InstantiatingWatchdogProvider.java | 2 ++ .../main/java/com/google/api/gax/rpc/Watchdog.java | 6 +++--- .../com/google/api/gax/rpc/ClientContextTest.java | 6 +++++- .../com/google/api/gax/rpc/ClientSettingsTest.java | 6 +++++- .../api/gax/rpc/FixedWatchdogProviderTest.java | 14 ++++++++++++-- repositories.bzl | 8 ++++++++ 8 files changed, 39 insertions(+), 8 deletions(-) diff --git a/gax-grpc/BUILD.bazel b/gax-grpc/BUILD.bazel index 29fac3b41..53b09f14f 100644 --- a/gax-grpc/BUILD.bazel +++ b/gax-grpc/BUILD.bazel @@ -24,6 +24,7 @@ _COMPILE_DEPS = [ "@com_google_http_client_google_http_client//jar", "@io_grpc_grpc_java//context:context", "@io_grpc_grpc_netty_shaded//jar", + "@io_grpc_grpc_grpclb//jar", "@io_grpc_grpc_java//alts:alts", "@io_netty_netty_tcnative_boringssl_static//jar", "@javax_annotation_javax_annotation_api//jar", diff --git a/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java b/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java index e22f14122..8d790a759 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java +++ b/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java @@ -31,13 +31,15 @@ import com.google.api.core.ApiClock; import com.google.api.core.BetaApi; +import com.google.api.core.InternalApi; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.threeten.bp.Duration; @BetaApi("The surface for streaming is not stable yet and may change in the future.") -public class FixedWatchdogProvider implements WatchdogProvider { +@InternalApi +public final class FixedWatchdogProvider implements WatchdogProvider { @Nullable private final Watchdog watchdog; public static WatchdogProvider create(Watchdog watchdog) { diff --git a/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java b/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java index b33c57048..555329cdb 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java +++ b/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java @@ -31,6 +31,7 @@ import com.google.api.core.ApiClock; import com.google.api.core.BetaApi; +import com.google.api.core.InternalApi; import com.google.common.base.Preconditions; import java.util.concurrent.ScheduledExecutorService; import javax.annotation.Nonnull; @@ -38,6 +39,7 @@ import org.threeten.bp.Duration; @BetaApi("The surface for streaming is not stable yet and may change in the future.") +@InternalApi public final class InstantiatingWatchdogProvider implements WatchdogProvider { @Nullable private final ApiClock clock; @Nullable private final ScheduledExecutorService executor; diff --git a/gax/src/main/java/com/google/api/gax/rpc/Watchdog.java b/gax/src/main/java/com/google/api/gax/rpc/Watchdog.java index 038bb1f3e..62ff8c4ae 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/Watchdog.java +++ b/gax/src/main/java/com/google/api/gax/rpc/Watchdog.java @@ -30,7 +30,7 @@ package com.google.api.gax.rpc; import com.google.api.core.ApiClock; -import com.google.api.core.InternalApi; +import com.google.api.core.BetaApi; import com.google.api.gax.core.BackgroundResource; import com.google.common.base.Preconditions; import java.util.Iterator; @@ -59,8 +59,8 @@ * had no outstanding demand. Duration.ZERO disables the timeout. * */ -@InternalApi -public class Watchdog implements Runnable, BackgroundResource { +@BetaApi +public final class Watchdog implements Runnable, BackgroundResource { // Dummy value to convert the ConcurrentHashMap into a Set private static Object PRESENT = new Object(); private final ConcurrentHashMap openStreams = new ConcurrentHashMap<>(); diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java index 17529fcb2..fb153c4b5 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java @@ -230,7 +230,11 @@ private void runTest( null); Credentials credentials = Mockito.mock(Credentials.class); ApiClock clock = Mockito.mock(ApiClock.class); - Watchdog watchdog = Mockito.mock(Watchdog.class); + Watchdog watchdog = + Watchdog.create( + Mockito.mock(ApiClock.class), + Duration.ZERO, + Mockito.mock(ScheduledExecutorService.class)); Duration watchdogCheckInterval = Duration.ofSeconds(11); builder.setExecutorProvider(executorProvider); diff --git a/gax/src/test/java/com/google/api/gax/rpc/ClientSettingsTest.java b/gax/src/test/java/com/google/api/gax/rpc/ClientSettingsTest.java index ef5111e83..ee490107d 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/ClientSettingsTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/ClientSettingsTest.java @@ -142,7 +142,11 @@ public void testBuilderFromClientContext() throws Exception { ApiClock clock = Mockito.mock(ApiClock.class); ApiCallContext callContext = FakeCallContext.createDefault(); Map headers = Collections.singletonMap("spiffykey", "spiffyvalue"); - Watchdog watchdog = Mockito.mock(Watchdog.class); + Watchdog watchdog = + Watchdog.create( + Mockito.mock(ApiClock.class), + Duration.ZERO, + Mockito.mock(ScheduledExecutorService.class)); Duration watchdogCheckInterval = Duration.ofSeconds(12); ClientContext clientContext = diff --git a/gax/src/test/java/com/google/api/gax/rpc/FixedWatchdogProviderTest.java b/gax/src/test/java/com/google/api/gax/rpc/FixedWatchdogProviderTest.java index de1fd43ab..9fb965937 100644 --- a/gax/src/test/java/com/google/api/gax/rpc/FixedWatchdogProviderTest.java +++ b/gax/src/test/java/com/google/api/gax/rpc/FixedWatchdogProviderTest.java @@ -49,14 +49,24 @@ public void testNull() { @Test public void testSameInstance() { - Watchdog watchdog = Mockito.mock(Watchdog.class); + Watchdog watchdog = + Watchdog.create( + Mockito.mock(ApiClock.class), + Duration.ZERO, + Mockito.mock(ScheduledExecutorService.class)); + WatchdogProvider provider = FixedWatchdogProvider.create(watchdog); assertThat(provider.getWatchdog()).isSameInstanceAs(watchdog); } @Test public void testNoModifications() { - WatchdogProvider provider = FixedWatchdogProvider.create(Mockito.mock(Watchdog.class)); + Watchdog watchdog = + Watchdog.create( + Mockito.mock(ApiClock.class), + Duration.ZERO, + Mockito.mock(ScheduledExecutorService.class)); + WatchdogProvider provider = FixedWatchdogProvider.create(watchdog); assertThat(provider.needsCheckInterval()).isFalse(); assertThat(provider.needsClock()).isFalse(); diff --git a/repositories.bzl b/repositories.bzl index 914b65442..4d9a429ff 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -79,6 +79,14 @@ def com_google_api_gax_java_repositories(): licenses = ["notice", "reciprocal"], ) + _maybe( + jvm_maven_import_external, + name = "io_grpc_grpc_grpclb", + artifact = "io.grpc:grpc-grpclb:%s" % PROPERTIES["version.io_grpc"], + server_urls = ["https://repo.maven.apache.org/maven2/", "http://repo1.maven.org/maven2/"], + licenses = ["notice", "reciprocal"], + ) + _maybe( jvm_maven_import_external, name = "google_java_format_all_deps", From e37713f8c4095b7797b17e95a5607ca633e19cd5 Mon Sep 17 00:00:00 2001 From: vam-google Date: Fri, 5 Jun 2020 10:40:49 -0700 Subject: [PATCH 2/2] address PR feedback --- .../java/com/google/api/gax/rpc/FixedWatchdogProvider.java | 7 +++++++ .../google/api/gax/rpc/InstantiatingWatchdogProvider.java | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java b/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java index 8d790a759..c958c66f6 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java +++ b/gax/src/main/java/com/google/api/gax/rpc/FixedWatchdogProvider.java @@ -37,6 +37,13 @@ import javax.annotation.Nullable; import org.threeten.bp.Duration; +/** + * A watchdog provider which always returns the same watchdog instance provided to the provider + * during construction. + * + *

This is the internal class and is public only for technical reasons. It may change any time + * without notice, please do not depend on it explicitly. + */ @BetaApi("The surface for streaming is not stable yet and may change in the future.") @InternalApi public final class FixedWatchdogProvider implements WatchdogProvider { diff --git a/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java b/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java index 555329cdb..b63d0ca99 100644 --- a/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java +++ b/gax/src/main/java/com/google/api/gax/rpc/InstantiatingWatchdogProvider.java @@ -38,6 +38,12 @@ import javax.annotation.Nullable; import org.threeten.bp.Duration; +/** + * A watchdog provider which instantiates a new provider on every request. + * + *

This is the internal class and is public only for technical reasons. It may change any time + * without notice, please do not depend on it explicitly. + */ @BetaApi("The surface for streaming is not stable yet and may change in the future.") @InternalApi public final class InstantiatingWatchdogProvider implements WatchdogProvider {