From a18eca898f13c967021ffc27698584126f6e6cc1 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Thu, 14 Nov 2019 16:29:03 -0800 Subject: [PATCH 1/4] Added API for accessing channel logger on NameResolver.Args. --- api/src/main/java/io/grpc/NameResolver.java | 44 ++++++++++++++++++- .../test/java/io/grpc/NameResolverTest.java | 4 ++ .../io/grpc/internal/ManagedChannelImpl.java | 13 +++--- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 463c44eea9d..c20eb784fb2 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -408,10 +408,21 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") public static final class Args { + private static final ChannelLogger NOOP_CHANNEL_LOGGER = new ChannelLogger() { + @Override + public void log(ChannelLogLevel level, String message) { + } + + @Override + public void log(ChannelLogLevel level, String messageFormat, Object... args) { + } + }; + private final int defaultPort; private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; + private final ChannelLogger channelLogger; @Nullable private final Executor executor; Args( @@ -419,11 +430,17 @@ public static final class Args { ProxyDetector proxyDetector, SynchronizationContext syncContext, ServiceConfigParser serviceConfigParser, + @Nullable ChannelLogger channelLogger, @Nullable Executor executor) { this.defaultPort = checkNotNull(defaultPort, "defaultPort not set"); this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); this.syncContext = checkNotNull(syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); + if (channelLogger == null) { + this.channelLogger = NOOP_CHANNEL_LOGGER; + } else { + this.channelLogger = channelLogger; + } this.executor = executor; } @@ -466,6 +483,15 @@ public ServiceConfigParser getServiceConfigParser() { return serviceConfigParser; } + /** + * Returns the {@link ChannelLogger} for the Channel served by this NameResolver. + * + * @since 1.26.0 + */ + public ChannelLogger getChannelLogger() { + return channelLogger; + } + /** * Returns the Executor on which this resolver should execute long-running or I/O bound work. * Null if no Executor was set. @@ -485,6 +511,7 @@ public String toString() { .add("proxyDetector", proxyDetector) .add("syncContext", syncContext) .add("serviceConfigParser", serviceConfigParser) + .add("channelLogger", channelLogger) .add("executor", executor) .toString(); } @@ -500,6 +527,7 @@ public Builder toBuilder() { builder.setProxyDetector(proxyDetector); builder.setSynchronizationContext(syncContext); builder.setServiceConfigParser(serviceConfigParser); + builder.setChannelLogger(channelLogger); builder.setOffloadExecutor(executor); return builder; } @@ -523,6 +551,7 @@ public static final class Builder { private ProxyDetector proxyDetector; private SynchronizationContext syncContext; private ServiceConfigParser serviceConfigParser; + private ChannelLogger channelLogger; private Executor executor; Builder() { @@ -568,6 +597,16 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) { return this; } + /** + * See {@link Args#getChannelLogger}. This is an optional field. + * + * @since 1.26.0 + */ + public Builder setChannelLogger(ChannelLogger channelLogger) { + this.channelLogger = checkNotNull(channelLogger); + return this; + } + /** * See {@link Args#getOffloadExecutor}. This is an optional field. * @@ -585,7 +624,10 @@ public Builder setOffloadExecutor(Executor executor) { * @since 1.21.0 */ public Args build() { - return new Args(defaultPort, proxyDetector, syncContext, serviceConfigParser, executor); + return + new Args( + defaultPort, proxyDetector, syncContext, serviceConfigParser, + channelLogger, executor); } } } diff --git a/api/src/test/java/io/grpc/NameResolverTest.java b/api/src/test/java/io/grpc/NameResolverTest.java index 3206f4be4ae..828047443a4 100644 --- a/api/src/test/java/io/grpc/NameResolverTest.java +++ b/api/src/test/java/io/grpc/NameResolverTest.java @@ -45,6 +45,7 @@ public class NameResolverTest { private final SynchronizationContext syncContext = new SynchronizationContext(mock(UncaughtExceptionHandler.class)); private final ServiceConfigParser parser = mock(ServiceConfigParser.class); + private final ChannelLogger channelLogger = mock(ChannelLogger.class); private final Executor executor = Executors.newSingleThreadExecutor(); private URI uri; private final NameResolver nameResolver = mock(NameResolver.class); @@ -61,6 +62,7 @@ public void args() { assertThat(args.getProxyDetector()).isSameInstanceAs(proxyDetector); assertThat(args.getSynchronizationContext()).isSameInstanceAs(syncContext); assertThat(args.getServiceConfigParser()).isSameInstanceAs(parser); + assertThat(args.getChannelLogger()).isSameInstanceAs(channelLogger); assertThat(args.getOffloadExecutor()).isSameInstanceAs(executor); NameResolver.Args args2 = args.toBuilder().build(); @@ -68,6 +70,7 @@ public void args() { assertThat(args2.getProxyDetector()).isSameInstanceAs(proxyDetector); assertThat(args2.getSynchronizationContext()).isSameInstanceAs(syncContext); assertThat(args2.getServiceConfigParser()).isSameInstanceAs(parser); + assertThat(args2.getChannelLogger()).isSameInstanceAs(channelLogger); assertThat(args2.getOffloadExecutor()).isSameInstanceAs(executor); assertThat(args2).isNotSameInstanceAs(args); @@ -251,6 +254,7 @@ private NameResolver.Args createArgs() { .setProxyDetector(proxyDetector) .setSynchronizationContext(syncContext) .setServiceConfigParser(parser) + .setChannelLogger(channelLogger) .setOffloadExecutor(executor) .build(); } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 560f8cba03b..de3a472501c 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -561,6 +561,12 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new final TimeProvider timeProvider) { this.target = checkNotNull(builder.target, "target"); this.logId = InternalLogId.allocate("Channel", target); + this.timeProvider = checkNotNull(timeProvider, "timeProvider"); + maxTraceEvents = builder.maxTraceEvents; + channelTracer = new ChannelTracer( + logId, builder.maxTraceEvents, timeProvider.currentTimeNanos(), + "Channel for '" + target + "'"); + channelLogger = new ChannelLoggerImpl(channelTracer, timeProvider); this.nameResolverFactory = builder.getNameResolverFactory(); ProxyDetector proxyDetector = builder.proxyDetector != null ? builder.proxyDetector : GrpcUtil.DEFAULT_PROXY_DETECTOR; @@ -581,6 +587,7 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new builder.maxRetryAttempts, builder.maxHedgedAttempts, loadBalancerFactory)) + .setChannelLogger(channelLogger) .setOffloadExecutor( // Avoid creating the offloadExecutor until it is first used new Executor() { @@ -591,12 +598,6 @@ public void execute(Runnable command) { }) .build(); this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverArgs); - this.timeProvider = checkNotNull(timeProvider, "timeProvider"); - maxTraceEvents = builder.maxTraceEvents; - channelTracer = new ChannelTracer( - logId, builder.maxTraceEvents, timeProvider.currentTimeNanos(), - "Channel for '" + target + "'"); - channelLogger = new ChannelLoggerImpl(channelTracer, timeProvider); this.executorPool = checkNotNull(builder.executorPool, "executorPool"); this.balancerRpcExecutorPool = checkNotNull(balancerRpcExecutorPool, "balancerRpcExecutorPool"); this.balancerRpcExecutorHolder = new ExecutorHolder(balancerRpcExecutorPool); From 11ba0179a1c2548ab26a99e1320f22658f35f60e Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 18 Nov 2019 15:10:44 -0800 Subject: [PATCH 2/4] Make ChannelLogger required in builder, but would only throw exception in its getter. --- api/src/main/java/io/grpc/NameResolver.java | 23 +++++-------------- .../io/grpc/NameResolverRegistryTest.java | 1 + .../internal/DnsNameResolverProviderTest.java | 2 ++ .../io/grpc/internal/DnsNameResolverTest.java | 4 ++++ ...ManagedChannelImplGetNameResolverTest.java | 2 ++ .../OverrideAuthorityNameResolverTest.java | 2 ++ .../grpc/xds/XdsNameResolverProviderTest.java | 2 ++ .../java/io/grpc/xds/XdsNameResolverTest.java | 2 ++ 8 files changed, 21 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index c20eb784fb2..f231af8ab3f 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -408,21 +408,11 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") public static final class Args { - private static final ChannelLogger NOOP_CHANNEL_LOGGER = new ChannelLogger() { - @Override - public void log(ChannelLogLevel level, String message) { - } - - @Override - public void log(ChannelLogLevel level, String messageFormat, Object... args) { - } - }; - private final int defaultPort; private final ProxyDetector proxyDetector; private final SynchronizationContext syncContext; private final ServiceConfigParser serviceConfigParser; - private final ChannelLogger channelLogger; + @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; Args( @@ -436,11 +426,7 @@ public void log(ChannelLogLevel level, String messageFormat, Object... args) { this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); this.syncContext = checkNotNull(syncContext, "syncContext not set"); this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); - if (channelLogger == null) { - this.channelLogger = NOOP_CHANNEL_LOGGER; - } else { - this.channelLogger = channelLogger; - } + this.channelLogger = channelLogger; this.executor = executor; } @@ -489,6 +475,9 @@ public ServiceConfigParser getServiceConfigParser() { * @since 1.26.0 */ public ChannelLogger getChannelLogger() { + if (channelLogger == null) { + throw new IllegalStateException("ChannelLogger is not set in Builder"); + } return channelLogger; } @@ -598,7 +587,7 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) { } /** - * See {@link Args#getChannelLogger}. This is an optional field. + * See {@link Args#getChannelLogger}. This is a required field. * * @since 1.26.0 */ diff --git a/api/src/test/java/io/grpc/NameResolverRegistryTest.java b/api/src/test/java/io/grpc/NameResolverRegistryTest.java index abbe10af2ac..f35f2f00b93 100644 --- a/api/src/test/java/io/grpc/NameResolverRegistryTest.java +++ b/api/src/test/java/io/grpc/NameResolverRegistryTest.java @@ -39,6 +39,7 @@ public class NameResolverRegistryTest { .setProxyDetector(mock(ProxyDetector.class)) .setSynchronizationContext(new SynchronizationContext(mock(UncaughtExceptionHandler.class))) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); @Test diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 3f5df9bdbbb..fa52a7d8511 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -24,6 +24,7 @@ import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.mock; +import io.grpc.ChannelLogger; import io.grpc.NameResolver; import io.grpc.NameResolver.ServiceConfigParser; import io.grpc.SynchronizationContext; @@ -47,6 +48,7 @@ public void uncaughtException(Thread t, Throwable e) { .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); private DnsNameResolverProvider provider = new DnsNameResolverProvider(); diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index 4edc2473dac..192d03d3341 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -36,6 +36,7 @@ import com.google.common.collect.Iterables; import com.google.common.net.InetAddresses; import com.google.common.testing.FakeTicker; +import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.NameResolver; @@ -115,6 +116,7 @@ public void uncaughtException(Thread t, Throwable e) { .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); private final DnsNameResolverProvider provider = new DnsNameResolverProvider(); @@ -175,6 +177,7 @@ private DnsNameResolver newResolver( .setProxyDetector(proxyDetector) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); return newResolver(name, stopwatch, isAndroid, args); } @@ -331,6 +334,7 @@ public void testExecutor_custom() throws Exception { .setProxyDetector(GrpcUtil.NOOP_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .setOffloadExecutor( new Executor() { @Override diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java index c620ce20dcb..c1d84eb76bf 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; +import io.grpc.ChannelLogger; import io.grpc.NameResolver; import io.grpc.NameResolver.Factory; import io.grpc.NameResolver.ServiceConfigParser; @@ -40,6 +41,7 @@ public class ManagedChannelImplGetNameResolverTest { .setProxyDetector(mock(ProxyDetector.class)) .setSynchronizationContext(new SynchronizationContext(mock(UncaughtExceptionHandler.class))) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); @Test diff --git a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java index 58b17d52fae..8d23ce3f884 100644 --- a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import io.grpc.ChannelLogger; import io.grpc.NameResolver; import io.grpc.NameResolver.ServiceConfigParser; import io.grpc.ProxyDetector; @@ -41,6 +42,7 @@ public class OverrideAuthorityNameResolverTest { .setProxyDetector(mock(ProxyDetector.class)) .setSynchronizationContext(new SynchronizationContext(mock(UncaughtExceptionHandler.class))) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); @Test diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java index 549c3c66a8f..275cf720739 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; +import io.grpc.ChannelLogger; import io.grpc.InternalServiceProviders; import io.grpc.NameResolver; import io.grpc.NameResolver.ServiceConfigParser; @@ -46,6 +47,7 @@ public void uncaughtException(Thread t, Throwable e) { .setProxyDetector(GrpcUtil.NOOP_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); private XdsNameResolverProvider provider = new XdsNameResolverProvider(); diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index d5811067fd0..d3efc826a5f 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import io.envoyproxy.envoy.api.v2.core.Node; +import io.grpc.ChannelLogger; import io.grpc.NameResolver; import io.grpc.NameResolver.ResolutionResult; import io.grpc.NameResolver.ServiceConfigParser; @@ -69,6 +70,7 @@ public void uncaughtException(Thread t, Throwable e) { .setProxyDetector(GrpcUtil.NOOP_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) .build(); private final XdsNameResolverProvider provider = new XdsNameResolverProvider(); From 495c4cebbd07b5a1309f28f5a58ec826e03aec78 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 18 Nov 2019 15:15:13 -0800 Subject: [PATCH 3/4] Added ExperimentalApi annotation. --- api/src/main/java/io/grpc/NameResolver.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index f231af8ab3f..ba678114895 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -474,6 +474,7 @@ public ServiceConfigParser getServiceConfigParser() { * * @since 1.26.0 */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/6438") public ChannelLogger getChannelLogger() { if (channelLogger == null) { throw new IllegalStateException("ChannelLogger is not set in Builder"); @@ -591,6 +592,7 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) { * * @since 1.26.0 */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/6438") public Builder setChannelLogger(ChannelLogger channelLogger) { this.channelLogger = checkNotNull(channelLogger); return this; From 1f0d13cc68f15518c5896315e7eab35468713534 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Mon, 18 Nov 2019 15:29:36 -0800 Subject: [PATCH 4/4] Removed requirement for channel logger in builder. --- api/src/main/java/io/grpc/NameResolver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index ba678114895..c89e8dd4b67 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -588,7 +588,7 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) { } /** - * See {@link Args#getChannelLogger}. This is a required field. + * See {@link Args#getChannelLogger}. * * @since 1.26.0 */