From 72210d4fb5781b2c192046bf756310f879371723 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Tue, 8 Sep 2020 18:00:44 -0400 Subject: [PATCH 01/11] core: remove last references to AbstractManagedChannelImplBuilder --- .../AbstractManagedChannelImplBuilder.java | 13 ----- .../io/grpc/internal/ManagedChannelImpl.java | 49 ++++++++++--------- .../internal/ManagedChannelImplBuilder.java | 23 +++++++++ ...va => ManagedChannelImplBuilderTest2.java} | 34 +++++-------- 4 files changed, 60 insertions(+), 59 deletions(-) rename core/src/test/java/io/grpc/internal/{AbstractManagedChannelImplBuilderTest.java => ManagedChannelImplBuilderTest2.java} (93%) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index aac6c25a8ee..dd320eeb868 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -28,7 +28,6 @@ import io.grpc.DecompressorRegistry; import io.grpc.EquivalentAddressGroup; import io.grpc.InternalChannelz; -import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; import io.grpc.NameResolverRegistry; @@ -507,18 +506,6 @@ protected String checkAuthority(String authority) { return GrpcUtil.checkAuthority(authority); } - @Override - public ManagedChannel build() { - return new ManagedChannelOrphanWrapper(new ManagedChannelImpl( - this, - buildTransportFactory(), - new ExponentialBackoffPolicy.Provider(), - SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR), - GrpcUtil.STOPWATCH_SUPPLIER, - getEffectiveInterceptors(), - TimeProvider.SYSTEM_TIME_PROVIDER)); - } - // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know // what should be the desired behavior for retry + stats/tracing. // TODO(zdapeng): FIX IT diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 494899b48c0..d8a0879cf01 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -44,6 +44,7 @@ import io.grpc.Context; import io.grpc.DecompressorRegistry; import io.grpc.EquivalentAddressGroup; +import io.grpc.ForwardingChannelBuilder; import io.grpc.InternalChannelz; import io.grpc.InternalChannelz.ChannelStats; import io.grpc.InternalChannelz.ChannelTrace; @@ -72,6 +73,8 @@ import io.grpc.SynchronizationContext.ScheduledHandle; import io.grpc.internal.AutoConfiguredLoadBalancerFactory.AutoConfiguredLoadBalancer; import io.grpc.internal.ClientCallImpl.ClientStreamProvider; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; +import io.grpc.internal.ManagedChannelImplBuilder.FixedPortProvider; import io.grpc.internal.ManagedChannelServiceConfig.MethodInfo; import io.grpc.internal.RetriableStream.ChannelBufferMeter; import io.grpc.internal.RetriableStream.Throttle; @@ -574,7 +577,7 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new private final Rescheduler idleTimer; ManagedChannelImpl( - AbstractManagedChannelImplBuilder builder, + ManagedChannelImplBuilder builder, ClientTransportFactory clientTransportFactory, BackoffPolicy.Provider backoffPolicyProvider, ObjectPool balancerRpcExecutorPool, @@ -661,7 +664,7 @@ public void execute(Runnable command) { } else { checkArgument( builder.idleTimeoutMillis - >= AbstractManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, + >= ManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, "invalid idleTimeoutMillis %s", builder.idleTimeoutMillis); this.idleTimeoutMillis = builder.idleTimeoutMillis; } @@ -1446,28 +1449,30 @@ public void run() { @Override public ManagedChannelBuilder createResolvingOobChannelBuilder(String target) { final class ResolvingOobChannelBuilder - extends AbstractManagedChannelImplBuilder { - int defaultPort = -1; + extends ForwardingChannelBuilder { + private final ManagedChannelImplBuilder managedChannelImplBuilder; ResolvingOobChannelBuilder(String target) { - super(target); + final class ResolvingOobChannelTransportFactoryBuilder extends + ClientTransportFactoryBuilderImpl {} + + managedChannelImplBuilder = new ManagedChannelImplBuilder(target, + new ResolvingOobChannelTransportFactoryBuilder(), + new FixedPortProvider(nameResolverArgs.getDefaultPort())); + managedChannelImplBuilder.executorPool = executorPool; + managedChannelImplBuilder.offloadExecutorPool = offloadExecutorHolder.pool; } @Override - public int getDefaultPort() { - return defaultPort; - } - - @Override - protected ClientTransportFactory buildTransportFactory() { - throw new UnsupportedOperationException(); + protected ManagedChannelBuilder delegate() { + return managedChannelImplBuilder; } @Override public ManagedChannel build() { // TODO(creamsoup) prevent main channel to shutdown if oob channel is not terminated return new ManagedChannelImpl( - this, + managedChannelImplBuilder, transportFactory, backoffPolicyProvider, balancerRpcExecutorPool, @@ -1479,17 +1484,15 @@ public ManagedChannel build() { checkState(!terminated, "Channel is terminated"); - ResolvingOobChannelBuilder builder = new ResolvingOobChannelBuilder(target); - builder.offloadExecutorPool = offloadExecutorHolder.pool; - builder.overrideAuthority(getAuthority()); @SuppressWarnings("deprecation") - ResolvingOobChannelBuilder unused = builder.nameResolverFactory(nameResolverFactory); - builder.executorPool = executorPool; - builder.maxTraceEvents = maxTraceEvents; - builder.proxyDetector = nameResolverArgs.getProxyDetector(); - builder.defaultPort = nameResolverArgs.getDefaultPort(); - builder.userAgent = userAgent; - return builder; + ResolvingOobChannelBuilder builder = new ResolvingOobChannelBuilder(target) + .nameResolverFactory(nameResolverFactory); + + return builder + .overrideAuthority(getAuthority()) + .maxTraceEvents(maxTraceEvents) + .proxyDetector(nameResolverArgs.getProxyDetector()) + .userAgent(userAgent); } @Override diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index d2807df200b..1043cc35984 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -17,6 +17,7 @@ package io.grpc.internal; import com.google.common.base.Preconditions; +import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import java.net.SocketAddress; import java.util.concurrent.Executor; @@ -38,6 +39,16 @@ public interface ClientTransportFactoryBuilder { ClientTransportFactory buildClientTransportFactory(); } + /** + * TODO(sergiitk): javadoc. + */ + public static class ClientTransportFactoryBuilderImpl implements ClientTransportFactoryBuilder { + @Override + public ClientTransportFactory buildClientTransportFactory() { + throw new UnsupportedOperationException(); + } + } + /** * An interface for Transport implementors to provide a default port to {@link * io.grpc.NameResolver} for use in cases where the target string doesn't include a port. The @@ -122,6 +133,18 @@ protected int getDefaultPort() { return channelBuilderDefaultPortProvider.getDefaultPort(); } + @Override + public ManagedChannel build() { + return new ManagedChannelOrphanWrapper(new ManagedChannelImpl( + this, + buildTransportFactory(), + new ExponentialBackoffPolicy.Provider(), + SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR), + GrpcUtil.STOPWATCH_SUPPLIER, + getEffectiveInterceptors(), + TimeProvider.SYSTEM_TIME_PROVIDER)); + } + /** Disable the check whether the authority is valid. */ public ManagedChannelImplBuilder disableCheckAuthority() { authorityCheckerDisabled = true; diff --git a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java similarity index 93% rename from core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java rename to core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java index 2ac49c48ce3..8e6082c943b 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java @@ -35,6 +35,7 @@ import io.grpc.DecompressorRegistry; import io.grpc.MethodDescriptor; import io.grpc.NameResolver; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; import java.net.InetSocketAddress; import java.net.SocketAddress; import java.net.URI; @@ -51,9 +52,9 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Unit tests for {@link AbstractManagedChannelImplBuilder}. */ +/** Unit tests for {@link ManagedChannelImplBuilder}. */ @RunWith(JUnit4.class) -public class AbstractManagedChannelImplBuilderTest { +public class ManagedChannelImplBuilderTest2 { @Rule public final ExpectedException thrown = ExpectedException.none(); @@ -67,8 +68,10 @@ public ClientCall interceptCall( } }; - private final Builder builder = new Builder("fake"); - private final Builder directAddressBuilder = new Builder(new SocketAddress(){}, "fake"); + private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", + new ClientTransportFactoryBuilderImpl(), null); + private final ManagedChannelImplBuilder directAddressBuilder = new ManagedChannelImplBuilder( + new SocketAddress() {}, "fake", new ClientTransportFactoryBuilderImpl(), null); @Test public void executor_default() { @@ -271,7 +274,7 @@ public void overrideAuthority_getNameResolverFactory() { public void makeTargetStringForDirectAddress_scopedIpv6() throws Exception { InetSocketAddress address = new InetSocketAddress("0:0:0:0:0:0:0:0%0", 10005); assertEquals("/0:0:0:0:0:0:0:0%0:10005", address.toString()); - String target = AbstractManagedChannelImplBuilder.makeTargetStringForDirectAddress(address); + String target = ManagedChannelImplBuilder.makeTargetStringForDirectAddress(address); URI uri = new URI(target); assertEquals("directaddress:////0:0:0:0:0:0:0:0%250:10005", target); assertEquals(target, uri.toString()); @@ -322,13 +325,13 @@ public void getEffectiveInterceptors_disableBoth() { @Test public void idleTimeout() { - assertEquals(AbstractManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, + assertEquals(ManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, builder.getIdleTimeoutMillis()); builder.idleTimeout(Long.MAX_VALUE, TimeUnit.DAYS); assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); - builder.idleTimeout(AbstractManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, + builder.idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS); assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); @@ -340,7 +343,7 @@ public void idleTimeout() { } builder.idleTimeout(1, TimeUnit.NANOSECONDS); - assertEquals(AbstractManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, + assertEquals(ManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, builder.getIdleTimeoutMillis()); builder.idleTimeout(30, TimeUnit.SECONDS); @@ -464,19 +467,4 @@ public void disableNameResolverServiceConfig() { builder.disableServiceConfigLookUp(); assertThat(builder.lookUpServiceConfig).isFalse(); } - - static class Builder extends AbstractManagedChannelImplBuilder { - Builder(String target) { - super(target); - } - - Builder(SocketAddress directServerAddress, String authority) { - super(directServerAddress, authority); - } - - @Override - protected ClientTransportFactory buildTransportFactory() { - throw new UnsupportedOperationException(); - } - } } From e66dcf141e8e089e32e67bf28680d7fb6896ed7f Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Fri, 11 Sep 2020 19:14:50 -0400 Subject: [PATCH 02/11] Inline AbstractManagedChannelImplBuilder: impls of ManagedChannelBuilder --- .../AbstractManagedChannelImplBuilder.java | 355 +----------------- .../internal/ManagedChannelImplBuilder.java | 334 +++++++++++++++- 2 files changed, 327 insertions(+), 362 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index dd320eeb868..f838d944848 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -16,11 +16,8 @@ package io.grpc.internal; -import static com.google.common.base.Preconditions.checkArgument; - import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.util.concurrent.MoreExecutors; import io.grpc.Attributes; import io.grpc.BinaryLog; import io.grpc.ClientInterceptor; @@ -38,9 +35,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; @@ -86,13 +81,13 @@ public static ManagedChannelBuilder forTarget(String target) { */ static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1); - private static final ObjectPool DEFAULT_EXECUTOR_POOL = + protected static final ObjectPool DEFAULT_EXECUTOR_POOL = SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR); - private static final DecompressorRegistry DEFAULT_DECOMPRESSOR_REGISTRY = + protected static final DecompressorRegistry DEFAULT_DECOMPRESSOR_REGISTRY = DecompressorRegistry.getDefaultInstance(); - private static final CompressorRegistry DEFAULT_COMPRESSOR_REGISTRY = + protected static final CompressorRegistry DEFAULT_COMPRESSOR_REGISTRY = CompressorRegistry.getDefaultInstance(); private static final long DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES = 1L << 24; // 16M @@ -102,16 +97,16 @@ public static ManagedChannelBuilder forTarget(String target) { ObjectPool offloadExecutorPool = DEFAULT_EXECUTOR_POOL; - private final List interceptors = new ArrayList<>(); + protected final List interceptors = new ArrayList<>(); final NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); // Access via getter, which may perform authority override as needed - private NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); + protected NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); final String target; @Nullable - private final SocketAddress directServerAddress; + protected final SocketAddress directServerAddress; @Nullable String userAgent; @@ -149,7 +144,7 @@ public static ManagedChannelBuilder forTarget(String target) { protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); - private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; + protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; @Nullable BinaryLog binlog; @@ -157,28 +152,11 @@ public static ManagedChannelBuilder forTarget(String target) { @Nullable ProxyDetector proxyDetector; - /** - * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages - * larger than this limit is received it will not be processed and the RPC will fail with - * RESOURCE_EXHAUSTED. - */ - // Can be overridden by subclasses. - @Override - public T maxInboundMessageSize(int max) { - checkArgument(max >= 0, "negative max"); - maxInboundMessageSize = max; - return thisT(); - } - - protected final int maxInboundMessageSize() { - return maxInboundMessageSize; - } - - private boolean statsEnabled = true; - private boolean recordStartedRpcs = true; - private boolean recordFinishedRpcs = true; - private boolean recordRealTimeMetrics = false; - private boolean tracingEnabled = true; + protected boolean statsEnabled = true; + protected boolean recordStartedRpcs = true; + protected boolean recordFinishedRpcs = true; + protected boolean recordRealTimeMetrics = false; + protected boolean tracingEnabled = true; protected AbstractManagedChannelImplBuilder(String target) { this.target = Preconditions.checkNotNull(target, "target"); @@ -206,306 +184,6 @@ protected AbstractManagedChannelImplBuilder(SocketAddress directServerAddress, S this.nameResolverFactory = new DirectAddressNameResolverFactory(directServerAddress, authority); } - @Override - public final T directExecutor() { - return executor(MoreExecutors.directExecutor()); - } - - @Override - public final T executor(Executor executor) { - if (executor != null) { - this.executorPool = new FixedObjectPool<>(executor); - } else { - this.executorPool = DEFAULT_EXECUTOR_POOL; - } - return thisT(); - } - - @Override - public final T offloadExecutor(Executor executor) { - if (executor != null) { - this.offloadExecutorPool = new FixedObjectPool<>(executor); - } else { - this.offloadExecutorPool = DEFAULT_EXECUTOR_POOL; - } - return thisT(); - } - - @Override - public final T intercept(List interceptors) { - this.interceptors.addAll(interceptors); - return thisT(); - } - - @Override - public final T intercept(ClientInterceptor... interceptors) { - return intercept(Arrays.asList(interceptors)); - } - - @Deprecated - @Override - public final T nameResolverFactory(NameResolver.Factory resolverFactory) { - Preconditions.checkState(directServerAddress == null, - "directServerAddress is set (%s), which forbids the use of NameResolverFactory", - directServerAddress); - if (resolverFactory != null) { - this.nameResolverFactory = resolverFactory; - } else { - this.nameResolverFactory = nameResolverRegistry.asFactory(); - } - return thisT(); - } - - @Override - public final T defaultLoadBalancingPolicy(String policy) { - Preconditions.checkState(directServerAddress == null, - "directServerAddress is set (%s), which forbids the use of load-balancing policy", - directServerAddress); - Preconditions.checkArgument(policy != null, "policy cannot be null"); - this.defaultLbPolicy = policy; - return thisT(); - } - - @Override - public final T enableFullStreamDecompression() { - this.fullStreamDecompression = true; - return thisT(); - } - - @Override - public final T decompressorRegistry(DecompressorRegistry registry) { - if (registry != null) { - this.decompressorRegistry = registry; - } else { - this.decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; - } - return thisT(); - } - - @Override - public final T compressorRegistry(CompressorRegistry registry) { - if (registry != null) { - this.compressorRegistry = registry; - } else { - this.compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; - } - return thisT(); - } - - @Override - public final T userAgent(@Nullable String userAgent) { - this.userAgent = userAgent; - return thisT(); - } - - @Override - public final T overrideAuthority(String authority) { - this.authorityOverride = checkAuthority(authority); - return thisT(); - } - - @Override - public final T idleTimeout(long value, TimeUnit unit) { - checkArgument(value > 0, "idle timeout is %s, but must be positive", value); - // We convert to the largest unit to avoid overflow - if (unit.toDays(value) >= IDLE_MODE_MAX_TIMEOUT_DAYS) { - // This disables idle mode - this.idleTimeoutMillis = ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE; - } else { - this.idleTimeoutMillis = Math.max(unit.toMillis(value), IDLE_MODE_MIN_TIMEOUT_MILLIS); - } - return thisT(); - } - - @Override - public final T maxRetryAttempts(int maxRetryAttempts) { - this.maxRetryAttempts = maxRetryAttempts; - return thisT(); - } - - @Override - public final T maxHedgedAttempts(int maxHedgedAttempts) { - this.maxHedgedAttempts = maxHedgedAttempts; - return thisT(); - } - - @Override - public final T retryBufferSize(long bytes) { - checkArgument(bytes > 0L, "retry buffer size must be positive"); - retryBufferSize = bytes; - return thisT(); - } - - @Override - public final T perRpcBufferLimit(long bytes) { - checkArgument(bytes > 0L, "per RPC buffer limit must be positive"); - perRpcBufferLimit = bytes; - return thisT(); - } - - @Override - public final T disableRetry() { - retryEnabled = false; - return thisT(); - } - - @Override - public final T enableRetry() { - retryEnabled = true; - statsEnabled = false; - tracingEnabled = false; - return thisT(); - } - - @Override - public final T setBinaryLog(BinaryLog binlog) { - this.binlog = binlog; - return thisT(); - } - - @Override - public T maxTraceEvents(int maxTraceEvents) { - checkArgument(maxTraceEvents >= 0, "maxTraceEvents must be non-negative"); - this.maxTraceEvents = maxTraceEvents; - return thisT(); - } - - @Override - public T proxyDetector(@Nullable ProxyDetector proxyDetector) { - this.proxyDetector = proxyDetector; - return thisT(); - } - - @Override - public T defaultServiceConfig(@Nullable Map serviceConfig) { - // TODO(notcarl): use real parsing - defaultServiceConfig = checkMapEntryTypes(serviceConfig); - return thisT(); - } - - @Nullable - private static Map checkMapEntryTypes(@Nullable Map map) { - if (map == null) { - return null; - } - // Not using ImmutableMap.Builder because of extra guava dependency for Android. - Map parsedMap = new LinkedHashMap<>(); - for (Map.Entry entry : map.entrySet()) { - checkArgument( - entry.getKey() instanceof String, - "The key of the entry '%s' is not of String type", entry); - - String key = (String) entry.getKey(); - Object value = entry.getValue(); - if (value == null) { - parsedMap.put(key, null); - } else if (value instanceof Map) { - parsedMap.put(key, checkMapEntryTypes((Map) value)); - } else if (value instanceof List) { - parsedMap.put(key, checkListEntryTypes((List) value)); - } else if (value instanceof String) { - parsedMap.put(key, value); - } else if (value instanceof Double) { - parsedMap.put(key, value); - } else if (value instanceof Boolean) { - parsedMap.put(key, value); - } else { - throw new IllegalArgumentException( - "The value of the map entry '" + entry + "' is of type '" + value.getClass() - + "', which is not supported"); - } - } - return Collections.unmodifiableMap(parsedMap); - } - - private static List checkListEntryTypes(List list) { - List parsedList = new ArrayList<>(list.size()); - for (Object value : list) { - if (value == null) { - parsedList.add(null); - } else if (value instanceof Map) { - parsedList.add(checkMapEntryTypes((Map) value)); - } else if (value instanceof List) { - parsedList.add(checkListEntryTypes((List) value)); - } else if (value instanceof String) { - parsedList.add(value); - } else if (value instanceof Double) { - parsedList.add(value); - } else if (value instanceof Boolean) { - parsedList.add(value); - } else { - throw new IllegalArgumentException( - "The entry '" + value + "' is of type '" + value.getClass() - + "', which is not supported"); - } - } - return Collections.unmodifiableList(parsedList); - } - - @Override - public T disableServiceConfigLookUp() { - this.lookUpServiceConfig = false; - return thisT(); - } - - /** - * Disable or enable stats features. Enabled by default. - * - *

For the current release, calling {@code setStatsEnabled(true)} may have a side effect that - * disables retry. - */ - protected void setStatsEnabled(boolean value) { - statsEnabled = value; - } - - /** - * Disable or enable stats recording for RPC upstarts. Effective only if {@link - * #setStatsEnabled} is set to true. Enabled by default. - */ - protected void setStatsRecordStartedRpcs(boolean value) { - recordStartedRpcs = value; - } - - /** - * Disable or enable stats recording for RPC completions. Effective only if {@link - * #setStatsEnabled} is set to true. Enabled by default. - */ - protected void setStatsRecordFinishedRpcs(boolean value) { - recordFinishedRpcs = value; - } - - /** - * Disable or enable real-time metrics recording. Effective only if {@link #setStatsEnabled} is - * set to true. Disabled by default. - */ - protected void setStatsRecordRealTimeMetrics(boolean value) { - recordRealTimeMetrics = value; - } - - /** - * Disable or enable tracing features. Enabled by default. - * - *

For the current release, calling {@code setTracingEnabled(true)} may have a side effect that - * disables retry. - */ - protected void setTracingEnabled(boolean value) { - tracingEnabled = value; - } - - @VisibleForTesting - final long getIdleTimeoutMillis() { - return idleTimeoutMillis; - } - - /** - * Verifies the authority is valid. This method exists as an escape hatch for putting in an - * authority that is valid, but would fail the default validation provided by this - * implementation. - */ - protected String checkAuthority(String authority) { - return GrpcUtil.checkAuthority(authority); - } - // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know // what should be the desired behavior for retry + stats/tracing. // TODO(zdapeng): FIX IT @@ -639,15 +317,6 @@ public String getDefaultScheme() { } } - /** - * Returns the correctly typed version of the builder. - */ - private T thisT() { - @SuppressWarnings("unchecked") - T thisT = (T) this; - return thisT; - } - /** * Returns the internal offload executor pool for offloading tasks. */ diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 1043cc35984..de600d61890 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -16,11 +16,26 @@ package io.grpc.internal; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.MoreExecutors; +import io.grpc.BinaryLog; +import io.grpc.ClientInterceptor; +import io.grpc.CompressorRegistry; +import io.grpc.DecompressorRegistry; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; +import io.grpc.NameResolver; +import io.grpc.ProxyDetector; import java.net.SocketAddress; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; /** @@ -145,49 +160,330 @@ public ManagedChannel build() { TimeProvider.SYSTEM_TIME_PROVIDER)); } - /** Disable the check whether the authority is valid. */ - public ManagedChannelImplBuilder disableCheckAuthority() { - authorityCheckerDisabled = true; + @Override + public ManagedChannelImplBuilder directExecutor() { + return executor(MoreExecutors.directExecutor()); + } + + @Override + public ManagedChannelImplBuilder executor(Executor executor) { + if (executor != null) { + this.executorPool = new FixedObjectPool<>(executor); + } else { + this.executorPool = DEFAULT_EXECUTOR_POOL; + } return this; } - /** Enable previously disabled authority check. */ - public ManagedChannelImplBuilder enableCheckAuthority() { - authorityCheckerDisabled = false; + @Override + public ManagedChannelImplBuilder offloadExecutor(Executor executor) { + if (executor != null) { + this.offloadExecutorPool = new FixedObjectPool<>(executor); + } else { + this.offloadExecutorPool = DEFAULT_EXECUTOR_POOL; + } return this; } @Override - protected String checkAuthority(String authority) { - if (authorityCheckerDisabled) { - return authority; + public ManagedChannelImplBuilder intercept(List interceptors) { + this.interceptors.addAll(interceptors); + return this; + } + + @Override + public ManagedChannelImplBuilder intercept(ClientInterceptor... interceptors) { + return intercept(Arrays.asList(interceptors)); + } + + @Override + public ManagedChannelImplBuilder userAgent(@Nullable String userAgent) { + this.userAgent = userAgent; + return this; + } + + @Override + public ManagedChannelImplBuilder overrideAuthority(String authority) { + this.authorityOverride = checkAuthority(authority); + return this; + } + + @Deprecated + @Override + public ManagedChannelImplBuilder nameResolverFactory(NameResolver.Factory resolverFactory) { + Preconditions.checkState(directServerAddress == null, + "directServerAddress is set (%s), which forbids the use of NameResolverFactory", + directServerAddress); + if (resolverFactory != null) { + this.nameResolverFactory = resolverFactory; + } else { + this.nameResolverFactory = nameResolverRegistry.asFactory(); } - return super.checkAuthority(authority); + return this; } @Override - public void setStatsEnabled(boolean value) { - super.setStatsEnabled(value); + public ManagedChannelImplBuilder defaultLoadBalancingPolicy(String policy) { + Preconditions.checkState(directServerAddress == null, + "directServerAddress is set (%s), which forbids the use of load-balancing policy", + directServerAddress); + Preconditions.checkArgument(policy != null, "policy cannot be null"); + this.defaultLbPolicy = policy; + return this; } @Override - public void setStatsRecordStartedRpcs(boolean value) { - super.setStatsRecordStartedRpcs(value); + public ManagedChannelImplBuilder enableFullStreamDecompression() { + this.fullStreamDecompression = true; + return this; } @Override - public void setStatsRecordFinishedRpcs(boolean value) { - super.setStatsRecordFinishedRpcs(value); + public ManagedChannelImplBuilder decompressorRegistry(DecompressorRegistry registry) { + if (registry != null) { + this.decompressorRegistry = registry; + } else { + this.decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; + } + return this; } @Override - public void setStatsRecordRealTimeMetrics(boolean value) { - super.setStatsRecordRealTimeMetrics(value); + public ManagedChannelImplBuilder compressorRegistry(CompressorRegistry registry) { + if (registry != null) { + this.compressorRegistry = registry; + } else { + this.compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; + } + return this; + } + + @Override + public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { + Preconditions.checkArgument(value > 0, "idle timeout is %s, but must be positive", value); + // We convert to the largest unit to avoid overflow + if (unit.toDays(value) >= IDLE_MODE_MAX_TIMEOUT_DAYS) { + // This disables idle mode + this.idleTimeoutMillis = ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE; + } else { + this.idleTimeoutMillis = Math.max(unit.toMillis(value), IDLE_MODE_MIN_TIMEOUT_MILLIS); + } + return this; + } + + /** + * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages + * larger than this limit is received it will not be processed and the RPC will fail with + * RESOURCE_EXHAUSTED. + */ + @Override + public ManagedChannelImplBuilder maxInboundMessageSize(int max) { + Preconditions.checkArgument(max >= 0, "negative max"); + maxInboundMessageSize = max; + return this; + } + + @Override + public ManagedChannelImplBuilder maxRetryAttempts(int maxRetryAttempts) { + this.maxRetryAttempts = maxRetryAttempts; + return this; + } + + @Override + public ManagedChannelImplBuilder maxHedgedAttempts(int maxHedgedAttempts) { + this.maxHedgedAttempts = maxHedgedAttempts; + return this; + } + + @Override + public ManagedChannelImplBuilder retryBufferSize(long bytes) { + Preconditions.checkArgument(bytes > 0L, "retry buffer size must be positive"); + retryBufferSize = bytes; + return this; + } + + @Override + public ManagedChannelImplBuilder perRpcBufferLimit(long bytes) { + Preconditions.checkArgument(bytes > 0L, "per RPC buffer limit must be positive"); + perRpcBufferLimit = bytes; + return this; + } + + @Override + public ManagedChannelImplBuilder disableRetry() { + retryEnabled = false; + return this; + } + + @Override + public ManagedChannelImplBuilder enableRetry() { + retryEnabled = true; + statsEnabled = false; + tracingEnabled = false; + return this; + } + + @Override + public ManagedChannelImplBuilder setBinaryLog(BinaryLog binlog) { + this.binlog = binlog; + return this; + } + + @Override + public ManagedChannelImplBuilder maxTraceEvents(int maxTraceEvents) { + Preconditions.checkArgument(maxTraceEvents >= 0, "maxTraceEvents must be non-negative"); + this.maxTraceEvents = maxTraceEvents; + return this; + } + + @Override + public ManagedChannelImplBuilder proxyDetector(@Nullable ProxyDetector proxyDetector) { + this.proxyDetector = proxyDetector; + return this; + } + + @Override + public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map serviceConfig) { + // TODO(notcarl): use real parsing + defaultServiceConfig = checkMapEntryTypes(serviceConfig); + return this; } @Override + public ManagedChannelImplBuilder disableServiceConfigLookUp() { + this.lookUpServiceConfig = false; + return this; + } + + @Nullable + private static Map checkMapEntryTypes(@Nullable Map map) { + if (map == null) { + return null; + } + // Not using ImmutableMap.Builder because of extra guava dependency for Android. + Map parsedMap = new LinkedHashMap<>(); + for (Map.Entry entry : map.entrySet()) { + Preconditions.checkArgument( + entry.getKey() instanceof String, + "The key of the entry '%s' is not of String type", entry); + + String key = (String) entry.getKey(); + Object value = entry.getValue(); + if (value == null) { + parsedMap.put(key, null); + } else if (value instanceof Map) { + parsedMap.put(key, checkMapEntryTypes((Map) value)); + } else if (value instanceof List) { + parsedMap.put(key, checkListEntryTypes((List) value)); + } else if (value instanceof String) { + parsedMap.put(key, value); + } else if (value instanceof Double) { + parsedMap.put(key, value); + } else if (value instanceof Boolean) { + parsedMap.put(key, value); + } else { + throw new IllegalArgumentException( + "The value of the map entry '" + entry + "' is of type '" + value.getClass() + + "', which is not supported"); + } + } + return Collections.unmodifiableMap(parsedMap); + } + + private static List checkListEntryTypes(List list) { + List parsedList = new ArrayList<>(list.size()); + for (Object value : list) { + if (value == null) { + parsedList.add(null); + } else if (value instanceof Map) { + parsedList.add(checkMapEntryTypes((Map) value)); + } else if (value instanceof List) { + parsedList.add(checkListEntryTypes((List) value)); + } else if (value instanceof String) { + parsedList.add(value); + } else if (value instanceof Double) { + parsedList.add(value); + } else if (value instanceof Boolean) { + parsedList.add(value); + } else { + throw new IllegalArgumentException( + "The entry '" + value + "' is of type '" + value.getClass() + + "', which is not supported"); + } + } + return Collections.unmodifiableList(parsedList); + } + + /** + * Disable or enable stats features. Enabled by default. + * + *

For the current release, calling {@code setStatsEnabled(true)} may have a side effect that + * disables retry. + */ + public void setStatsEnabled(boolean value) { + statsEnabled = value; + } + + /** + * Disable or enable stats recording for RPC upstarts. Effective only if {@link + * #setStatsEnabled} is set to true. Enabled by default. + */ + public void setStatsRecordStartedRpcs(boolean value) { + recordStartedRpcs = value; + } + + /** + * Disable or enable stats recording for RPC completions. Effective only if {@link + * #setStatsEnabled} is set to true. Enabled by default. + */ + public void setStatsRecordFinishedRpcs(boolean value) { + recordFinishedRpcs = value; + } + + /** + * Disable or enable real-time metrics recording. Effective only if {@link #setStatsEnabled} is + * set to true. Disabled by default. + */ + public void setStatsRecordRealTimeMetrics(boolean value) { + recordRealTimeMetrics = value; + } + + /** + * Disable or enable tracing features. Enabled by default. + * + *

For the current release, calling {@code setTracingEnabled(true)} may have a side effect that + * disables retry. + */ public void setTracingEnabled(boolean value) { - super.setTracingEnabled(value); + tracingEnabled = value; + } + + /** Disable the check whether the authority is valid. */ + public ManagedChannelImplBuilder disableCheckAuthority() { + authorityCheckerDisabled = true; + return this; + } + + /** Enable previously disabled authority check. */ + public ManagedChannelImplBuilder enableCheckAuthority() { + authorityCheckerDisabled = false; + return this; + } + + @VisibleForTesting + final long getIdleTimeoutMillis() { + return idleTimeoutMillis; + } + + /** + * Verifies the authority is valid. + */ + @VisibleForTesting + String checkAuthority(String authority) { + if (authorityCheckerDisabled) { + return authority; + } + return GrpcUtil.checkAuthority(authority); } @Override From cd5b9f1d98e47054b65e3f783a31c0180017c6e4 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 13:28:10 -0400 Subject: [PATCH 03/11] Inline AbstractManagedChannelImplBuilder: convert methods calling super() --- .../AbstractManagedChannelImplBuilder.java | 189 ---------------- .../internal/ManagedChannelImplBuilder.java | 203 ++++++++++++++++-- 2 files changed, 181 insertions(+), 211 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index f838d944848..9ff08e038a4 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -17,31 +17,20 @@ package io.grpc.internal; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; -import io.grpc.Attributes; import io.grpc.BinaryLog; import io.grpc.ClientInterceptor; import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; -import io.grpc.EquivalentAddressGroup; import io.grpc.InternalChannelz; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; -import java.net.SocketAddress; -import java.net.URI; -import java.net.URISyntaxException; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; -import java.util.logging.Level; -import java.util.logging.Logger; import javax.annotation.Nullable; /** @@ -51,11 +40,6 @@ */ public abstract class AbstractManagedChannelImplBuilder > extends ManagedChannelBuilder { - private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; - - private static final Logger log = - Logger.getLogger(AbstractManagedChannelImplBuilder.class.getName()); - public static ManagedChannelBuilder forAddress(String name, int port) { throw new UnsupportedOperationException("Subclass failed to hide static factory"); } @@ -103,11 +87,6 @@ public static ManagedChannelBuilder forTarget(String target) { // Access via getter, which may perform authority override as needed protected NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); - final String target; - - @Nullable - protected final SocketAddress directServerAddress; - @Nullable String userAgent; @@ -142,8 +121,6 @@ public static ManagedChannelBuilder forTarget(String target) { Map defaultServiceConfig; boolean lookUpServiceConfig = true; - protected TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); - protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; @Nullable @@ -157,170 +134,4 @@ public static ManagedChannelBuilder forTarget(String target) { protected boolean recordFinishedRpcs = true; protected boolean recordRealTimeMetrics = false; protected boolean tracingEnabled = true; - - protected AbstractManagedChannelImplBuilder(String target) { - this.target = Preconditions.checkNotNull(target, "target"); - this.directServerAddress = null; - } - - /** - * Returns a target string for the SocketAddress. It is only used as a placeholder, because - * DirectAddressNameResolverFactory will not actually try to use it. However, it must be a valid - * URI. - */ - @VisibleForTesting - static String makeTargetStringForDirectAddress(SocketAddress address) { - try { - return new URI(DIRECT_ADDRESS_SCHEME, "", "/" + address, null).toString(); - } catch (URISyntaxException e) { - // It should not happen. - throw new RuntimeException(e); - } - } - - protected AbstractManagedChannelImplBuilder(SocketAddress directServerAddress, String authority) { - this.target = makeTargetStringForDirectAddress(directServerAddress); - this.directServerAddress = directServerAddress; - this.nameResolverFactory = new DirectAddressNameResolverFactory(directServerAddress, authority); - } - - // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know - // what should be the desired behavior for retry + stats/tracing. - // TODO(zdapeng): FIX IT - @VisibleForTesting - final List getEffectiveInterceptors() { - List effectiveInterceptors = - new ArrayList<>(this.interceptors); - temporarilyDisableRetry = false; - if (statsEnabled) { - temporarilyDisableRetry = true; - ClientInterceptor statsInterceptor = null; - try { - Class censusStatsAccessor = - Class.forName("io.grpc.census.InternalCensusStatsAccessor"); - Method getClientInterceptorMethod = - censusStatsAccessor.getDeclaredMethod( - "getClientInterceptor", - boolean.class, - boolean.class, - boolean.class); - statsInterceptor = - (ClientInterceptor) getClientInterceptorMethod - .invoke( - null, - recordStartedRpcs, - recordFinishedRpcs, - recordRealTimeMetrics); - } catch (ClassNotFoundException e) { - // Replace these separate catch statements with multicatch when Android min-API >= 19 - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (NoSuchMethodException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (IllegalAccessException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (InvocationTargetException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } - if (statsInterceptor != null) { - // First interceptor runs last (see ClientInterceptors.intercept()), so that no - // other interceptor can override the tracer factory we set in CallOptions. - effectiveInterceptors.add(0, statsInterceptor); - } - } - if (tracingEnabled) { - temporarilyDisableRetry = true; - ClientInterceptor tracingInterceptor = null; - try { - Class censusTracingAccessor = - Class.forName("io.grpc.census.InternalCensusTracingAccessor"); - Method getClientInterceptroMethod = - censusTracingAccessor.getDeclaredMethod("getClientInterceptor"); - tracingInterceptor = (ClientInterceptor) getClientInterceptroMethod.invoke(null); - } catch (ClassNotFoundException e) { - // Replace these separate catch statements with multicatch when Android min-API >= 19 - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (NoSuchMethodException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (IllegalAccessException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (InvocationTargetException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } - if (tracingInterceptor != null) { - effectiveInterceptors.add(0, tracingInterceptor); - } - } - return effectiveInterceptors; - } - - /** - * Subclasses should override this method to provide the {@link ClientTransportFactory} - * appropriate for this channel. This method is meant for Transport implementors and should not - * be used by normal users. - */ - protected abstract ClientTransportFactory buildTransportFactory(); - - /** - * Subclasses can override this method to provide a default port to {@link NameResolver} for use - * in cases where the target string doesn't include a port. The default implementation returns - * {@link GrpcUtil#DEFAULT_PORT_SSL}. - */ - protected int getDefaultPort() { - return GrpcUtil.DEFAULT_PORT_SSL; - } - - /** - * Returns a {@link NameResolver.Factory} for the channel. - */ - NameResolver.Factory getNameResolverFactory() { - if (authorityOverride == null) { - return nameResolverFactory; - } else { - return new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); - } - } - - private static class DirectAddressNameResolverFactory extends NameResolver.Factory { - final SocketAddress address; - final String authority; - - DirectAddressNameResolverFactory(SocketAddress address, String authority) { - this.address = address; - this.authority = authority; - } - - @Override - public NameResolver newNameResolver(URI notUsedUri, NameResolver.Args args) { - return new NameResolver() { - @Override - public String getServiceAuthority() { - return authority; - } - - @Override - public void start(Listener2 listener) { - listener.onResult( - ResolutionResult.newBuilder() - .setAddresses(Collections.singletonList(new EquivalentAddressGroup(address))) - .setAttributes(Attributes.EMPTY) - .build()); - } - - @Override - public void shutdown() {} - }; - } - - @Override - public String getDefaultScheme() { - return DIRECT_ADDRESS_SCHEME; - } - } - - /** - * Returns the internal offload executor pool for offloading tasks. - */ - protected ObjectPool getOffloadExecutorPool() { - return this.offloadExecutorPool; - } } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index de600d61890..63c5e2a950e 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -19,15 +19,21 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; +import io.grpc.Attributes; import io.grpc.BinaryLog; import io.grpc.ClientInterceptor; import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; +import io.grpc.EquivalentAddressGroup; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; import io.grpc.ProxyDetector; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.net.SocketAddress; +import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -36,6 +42,8 @@ import java.util.Map; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.Nullable; /** @@ -44,8 +52,6 @@ public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { - private boolean authorityCheckerDisabled; - /** * An interface for Transport implementors to provide the {@link ClientTransportFactory} * appropriate for the channel. @@ -89,16 +95,33 @@ public int getDefaultPort() { } } - private final class ManagedChannelDefaultPortProvider implements + private static final class ManagedChannelDefaultPortProvider implements ChannelBuilderDefaultPortProvider { @Override public int getDefaultPort() { - return ManagedChannelImplBuilder.super.getDefaultPort(); + return GrpcUtil.DEFAULT_PORT_SSL; } } + private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; + private static final Logger log = Logger.getLogger(ManagedChannelImplBuilder.class.getName()); + + final String target; + @Nullable + private final SocketAddress directServerAddress; private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; + private boolean authorityCheckerDisabled; + + public static ManagedChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException( + "ClientTransportFactoryBuilder is required, use a constructor"); + } + + public static ManagedChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException( + "ClientTransportFactoryBuilder is required, use a constructor"); + } /** * Creates a new managed channel builder with a target string, which can be either a valid {@link @@ -108,9 +131,10 @@ public int getDefaultPort() { public ManagedChannelImplBuilder(String target, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { - super(target); + this.target = Preconditions.checkNotNull(target, "target"); this.clientTransportFactoryBuilder = Preconditions .checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder"); + this.directServerAddress = null; if (channelBuilderDefaultPortProvider != null) { this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; @@ -127,9 +151,11 @@ public ManagedChannelImplBuilder(String target, public ManagedChannelImplBuilder(SocketAddress directServerAddress, String authority, ClientTransportFactoryBuilder clientTransportFactoryBuilder, @Nullable ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider) { - super(directServerAddress, authority); + this.target = makeTargetStringForDirectAddress(directServerAddress); this.clientTransportFactoryBuilder = Preconditions .checkNotNull(clientTransportFactoryBuilder, "clientTransportFactoryBuilder"); + this.directServerAddress = directServerAddress; + this.nameResolverFactory = new DirectAddressNameResolverFactory(directServerAddress, authority); if (channelBuilderDefaultPortProvider != null) { this.channelBuilderDefaultPortProvider = channelBuilderDefaultPortProvider; @@ -138,13 +164,35 @@ public ManagedChannelImplBuilder(SocketAddress directServerAddress, String autho } } - @Override - protected ClientTransportFactory buildTransportFactory() { + /** + * Returns a target string for the SocketAddress. It is only used as a placeholder, because + * DirectAddressNameResolverFactory will not actually try to use it. However, it must be a valid + * URI. + */ + @VisibleForTesting + static String makeTargetStringForDirectAddress(SocketAddress address) { + try { + return new URI(DIRECT_ADDRESS_SCHEME, "", "/" + address, null).toString(); + } catch (URISyntaxException e) { + // It should not happen. + throw new RuntimeException(e); + } + } + + /** + * Transport implementors must provide {@link ClientTransportFactoryBuilder} that returns {@link + * ClientTransportFactory} appropriate the channel. This method is meant for Transport + * implementors and should not be used by normal users. + */ + ClientTransportFactory buildTransportFactory() { return clientTransportFactoryBuilder.buildClientTransportFactory(); } - @Override - protected int getDefaultPort() { + /** + * Returns a default port to {@link NameResolver} for use in cases where the target string doesn't + * include a port. The default implementation returns {@link GrpcUtil#DEFAULT_PORT_SSL}. + */ + int getDefaultPort() { return channelBuilderDefaultPortProvider.getDefaultPort(); } @@ -272,8 +320,8 @@ public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { } /** - * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages - * larger than this limit is received it will not be processed and the RPC will fail with + * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages larger + * than this limit is received it will not be processed and the RPC will fail with * RESOURCE_EXHAUSTED. */ @Override @@ -425,8 +473,8 @@ public void setStatsEnabled(boolean value) { } /** - * Disable or enable stats recording for RPC upstarts. Effective only if {@link - * #setStatsEnabled} is set to true. Enabled by default. + * Disable or enable stats recording for RPC upstarts. Effective only if {@link #setStatsEnabled} + * is set to true. Enabled by default. */ public void setStatsRecordStartedRpcs(boolean value) { recordStartedRpcs = value; @@ -451,8 +499,8 @@ public void setStatsRecordRealTimeMetrics(boolean value) { /** * Disable or enable tracing features. Enabled by default. * - *

For the current release, calling {@code setTracingEnabled(true)} may have a side effect that - * disables retry. + *

For the current release, calling {@code setTracingEnabled(true)} may have a side effect + * that disables retry. */ public void setTracingEnabled(boolean value) { tracingEnabled = value; @@ -486,16 +534,127 @@ String checkAuthority(String authority) { return GrpcUtil.checkAuthority(authority); } - @Override + /** + * Returns the internal offload executor pool for offloading tasks. + */ public ObjectPool getOffloadExecutorPool() { - return super.getOffloadExecutorPool(); + return this.offloadExecutorPool; } - public static ManagedChannelBuilder forAddress(String name, int port) { - throw new UnsupportedOperationException("ClientTransportFactoryBuilder is required"); + /** + * Returns a {@link NameResolver.Factory} for the channel. + */ + NameResolver.Factory getNameResolverFactory() { + if (authorityOverride == null) { + return nameResolverFactory; + } else { + return new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); + } } - public static ManagedChannelBuilder forTarget(String target) { - throw new UnsupportedOperationException("ClientTransportFactoryBuilder is required"); + // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know + // what should be the desired behavior for retry + stats/tracing. + // TODO(zdapeng): FIX IT + @VisibleForTesting + final List getEffectiveInterceptors() { + List effectiveInterceptors = + new ArrayList<>(this.interceptors); + temporarilyDisableRetry = false; + if (statsEnabled) { + temporarilyDisableRetry = true; + ClientInterceptor statsInterceptor = null; + try { + Class censusStatsAccessor = + Class.forName("io.grpc.census.InternalCensusStatsAccessor"); + Method getClientInterceptorMethod = + censusStatsAccessor.getDeclaredMethod( + "getClientInterceptor", + boolean.class, + boolean.class, + boolean.class); + statsInterceptor = + (ClientInterceptor) getClientInterceptorMethod + .invoke( + null, + recordStartedRpcs, + recordFinishedRpcs, + recordRealTimeMetrics); + } catch (ClassNotFoundException e) { + // Replace these separate catch statements with multicatch when Android min-API >= 19 + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (NoSuchMethodException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (IllegalAccessException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (InvocationTargetException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } + if (statsInterceptor != null) { + // First interceptor runs last (see ClientInterceptors.intercept()), so that no + // other interceptor can override the tracer factory we set in CallOptions. + effectiveInterceptors.add(0, statsInterceptor); + } + } + if (tracingEnabled) { + temporarilyDisableRetry = true; + ClientInterceptor tracingInterceptor = null; + try { + Class censusTracingAccessor = + Class.forName("io.grpc.census.InternalCensusTracingAccessor"); + Method getClientInterceptroMethod = + censusTracingAccessor.getDeclaredMethod("getClientInterceptor"); + tracingInterceptor = (ClientInterceptor) getClientInterceptroMethod.invoke(null); + } catch (ClassNotFoundException e) { + // Replace these separate catch statements with multicatch when Android min-API >= 19 + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (NoSuchMethodException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (IllegalAccessException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (InvocationTargetException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } + if (tracingInterceptor != null) { + effectiveInterceptors.add(0, tracingInterceptor); + } + } + return effectiveInterceptors; + } + + private static class DirectAddressNameResolverFactory extends NameResolver.Factory { + final SocketAddress address; + final String authority; + + DirectAddressNameResolverFactory(SocketAddress address, String authority) { + this.address = address; + this.authority = authority; + } + + @Override + public NameResolver newNameResolver(URI notUsedUri, NameResolver.Args args) { + return new NameResolver() { + @Override + public String getServiceAuthority() { + return authority; + } + + @Override + public void start(Listener2 listener) { + listener.onResult( + ResolutionResult.newBuilder() + .setAddresses(Collections.singletonList(new EquivalentAddressGroup(address))) + .setAttributes(Attributes.EMPTY) + .build()); + } + + @Override + public void shutdown() {} + }; + } + + @Override + public String getDefaultScheme() { + return DIRECT_ADDRESS_SCHEME; + } } } From 3d96c6ad26224a72b84a97e9de822c2d4a868f99 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 15:18:44 -0400 Subject: [PATCH 04/11] Inline AbstractManagedChannelImplBuilder: move properties --- .../AbstractManagedChannelImplBuilder.java | 102 ---------- .../internal/ManagedChannelImplBuilder.java | 192 +++++++++++------- 2 files changed, 123 insertions(+), 171 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 9ff08e038a4..d78b1e80970 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -16,22 +16,7 @@ package io.grpc.internal; -import com.google.common.annotations.VisibleForTesting; -import io.grpc.BinaryLog; -import io.grpc.ClientInterceptor; -import io.grpc.CompressorRegistry; -import io.grpc.DecompressorRegistry; -import io.grpc.InternalChannelz; import io.grpc.ManagedChannelBuilder; -import io.grpc.NameResolver; -import io.grpc.NameResolverRegistry; -import io.grpc.ProxyDetector; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.concurrent.Executor; -import java.util.concurrent.TimeUnit; -import javax.annotation.Nullable; /** * Abstract base class for channel builders. @@ -47,91 +32,4 @@ public static ManagedChannelBuilder forAddress(String name, int port) { public static ManagedChannelBuilder forTarget(String target) { throw new UnsupportedOperationException("Subclass failed to hide static factory"); } - - /** - * An idle timeout larger than this would disable idle mode. - */ - @VisibleForTesting - static final long IDLE_MODE_MAX_TIMEOUT_DAYS = 30; - - /** - * The default idle timeout. - */ - @VisibleForTesting - static final long IDLE_MODE_DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(30); - - /** - * An idle timeout smaller than this would be capped to it. - */ - static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1); - - protected static final ObjectPool DEFAULT_EXECUTOR_POOL = - SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR); - - protected static final DecompressorRegistry DEFAULT_DECOMPRESSOR_REGISTRY = - DecompressorRegistry.getDefaultInstance(); - - protected static final CompressorRegistry DEFAULT_COMPRESSOR_REGISTRY = - CompressorRegistry.getDefaultInstance(); - - private static final long DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES = 1L << 24; // 16M - private static final long DEFAULT_PER_RPC_BUFFER_LIMIT_IN_BYTES = 1L << 20; // 1M - - ObjectPool executorPool = DEFAULT_EXECUTOR_POOL; - - ObjectPool offloadExecutorPool = DEFAULT_EXECUTOR_POOL; - - protected final List interceptors = new ArrayList<>(); - final NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); - - // Access via getter, which may perform authority override as needed - protected NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); - - @Nullable - String userAgent; - - @VisibleForTesting - @Nullable - String authorityOverride; - - String defaultLbPolicy = GrpcUtil.DEFAULT_LB_POLICY; - - boolean fullStreamDecompression; - - DecompressorRegistry decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; - - CompressorRegistry compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; - - long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS; - - int maxRetryAttempts = 5; - int maxHedgedAttempts = 5; - long retryBufferSize = DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES; - long perRpcBufferLimit = DEFAULT_PER_RPC_BUFFER_LIMIT_IN_BYTES; - boolean retryEnabled = false; // TODO(zdapeng): default to true - // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know - // what should be the desired behavior for retry + stats/tracing. - // TODO(zdapeng): delete me - boolean temporarilyDisableRetry; - - InternalChannelz channelz = InternalChannelz.instance(); - int maxTraceEvents; - - @Nullable - Map defaultServiceConfig; - boolean lookUpServiceConfig = true; - - protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; - - @Nullable - BinaryLog binlog; - - @Nullable - ProxyDetector proxyDetector; - - protected boolean statsEnabled = true; - protected boolean recordStartedRpcs = true; - protected boolean recordFinishedRpcs = true; - protected boolean recordRealTimeMetrics = false; - protected boolean tracingEnabled = true; } diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 63c5e2a950e..bfc18dfa5b0 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -25,9 +25,11 @@ import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; import io.grpc.EquivalentAddressGroup; +import io.grpc.InternalChannelz; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; +import io.grpc.NameResolverRegistry; import io.grpc.ProxyDetector; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; @@ -51,77 +53,80 @@ */ public final class ManagedChannelImplBuilder extends AbstractManagedChannelImplBuilder { + private static final Logger log = Logger.getLogger(ManagedChannelImplBuilder.class.getName()); + private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; + private static final long DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES = 1L << 24; // 16M + private static final long DEFAULT_PER_RPC_BUFFER_LIMIT_IN_BYTES = 1L << 20; // 1Ms - /** - * An interface for Transport implementors to provide the {@link ClientTransportFactory} - * appropriate for the channel. - */ - public interface ClientTransportFactoryBuilder { - ClientTransportFactory buildClientTransportFactory(); - } - - /** - * TODO(sergiitk): javadoc. - */ - public static class ClientTransportFactoryBuilderImpl implements ClientTransportFactoryBuilder { - @Override - public ClientTransportFactory buildClientTransportFactory() { - throw new UnsupportedOperationException(); - } - } - - /** - * An interface for Transport implementors to provide a default port to {@link - * io.grpc.NameResolver} for use in cases where the target string doesn't include a port. The - * default implementation returns {@link GrpcUtil#DEFAULT_PORT_SSL}. - */ - public interface ChannelBuilderDefaultPortProvider { - int getDefaultPort(); - } - - /** - * Default implementation of {@link ChannelBuilderDefaultPortProvider} that returns a fixed port. - */ - public static final class FixedPortProvider implements ChannelBuilderDefaultPortProvider { - private final int port; + private static final ObjectPool DEFAULT_EXECUTOR_POOL = + SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR); + private static final DecompressorRegistry DEFAULT_DECOMPRESSOR_REGISTRY = + DecompressorRegistry.getDefaultInstance(); + private static final CompressorRegistry DEFAULT_COMPRESSOR_REGISTRY = + CompressorRegistry.getDefaultInstance(); - public FixedPortProvider(int port) { - this.port = port; - } - @Override - public int getDefaultPort() { - return port; - } - } + /** An idle timeout smaller than this would be capped to it. */ + static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1); - private static final class ManagedChannelDefaultPortProvider implements - ChannelBuilderDefaultPortProvider { - @Override - public int getDefaultPort() { - return GrpcUtil.DEFAULT_PORT_SSL; - } - } + /** An idle timeout larger than this would disable idle mode. */ + @VisibleForTesting static final long IDLE_MODE_MAX_TIMEOUT_DAYS = 30; - private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; - private static final Logger log = Logger.getLogger(ManagedChannelImplBuilder.class.getName()); + /** The default idle timeout. */ + @VisibleForTesting + static final long IDLE_MODE_DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(30); final String target; @Nullable private final SocketAddress directServerAddress; private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; + private final List interceptors = new ArrayList<>(); + + @VisibleForTesting + @Nullable + String authorityOverride; private boolean authorityCheckerDisabled; + private boolean statsEnabled = true; + private boolean recordStartedRpcs = true; + private boolean recordFinishedRpcs = true; + private boolean recordRealTimeMetrics = false; + private boolean tracingEnabled = true; - public static ManagedChannelBuilder forAddress(String name, int port) { - throw new UnsupportedOperationException( - "ClientTransportFactoryBuilder is required, use a constructor"); - } + ObjectPool executorPool = DEFAULT_EXECUTOR_POOL; + ObjectPool offloadExecutorPool = DEFAULT_EXECUTOR_POOL; - public static ManagedChannelBuilder forTarget(String target) { - throw new UnsupportedOperationException( - "ClientTransportFactoryBuilder is required, use a constructor"); - } + DecompressorRegistry decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; + CompressorRegistry compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; + + final NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); + // Access via getter, which may perform authority override as needed + NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); + + int maxRetryAttempts = 5; + int maxHedgedAttempts = 5; + long retryBufferSize = DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES; + long perRpcBufferLimit = DEFAULT_PER_RPC_BUFFER_LIMIT_IN_BYTES; + boolean retryEnabled = false; // TODO(zdapeng): default to true + + long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS; + @Nullable + String userAgent; + String defaultLbPolicy = GrpcUtil.DEFAULT_LB_POLICY; + boolean fullStreamDecompression; + // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know + // what should be the desired behavior for retry + stats/tracing. + // TODO(zdapeng): delete me + boolean temporarilyDisableRetry; + int maxTraceEvents; + InternalChannelz channelz = InternalChannelz.instance(); + @Nullable + Map defaultServiceConfig; + boolean lookUpServiceConfig = true; + @Nullable + BinaryLog binlog; + @Nullable + ProxyDetector proxyDetector; /** * Creates a new managed channel builder with a target string, which can be either a valid {@link @@ -319,18 +324,6 @@ public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { return this; } - /** - * Sets the maximum message size allowed for a single gRPC frame. If an inbound messages larger - * than this limit is received it will not be processed and the RPC will fail with - * RESOURCE_EXHAUSTED. - */ - @Override - public ManagedChannelImplBuilder maxInboundMessageSize(int max) { - Preconditions.checkArgument(max >= 0, "negative max"); - maxInboundMessageSize = max; - return this; - } - @Override public ManagedChannelImplBuilder maxRetryAttempts(int maxRetryAttempts) { this.maxRetryAttempts = maxRetryAttempts; @@ -621,6 +614,67 @@ final List getEffectiveInterceptors() { return effectiveInterceptors; } + public static ManagedChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException( + "ClientTransportFactoryBuilder is required, use a constructor"); + } + + public static ManagedChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException( + "ClientTransportFactoryBuilder is required, use a constructor"); + } + + /** + * An interface for Transport implementors to provide the {@link ClientTransportFactory} + * appropriate for the channel. + */ + public interface ClientTransportFactoryBuilder { + ClientTransportFactory buildClientTransportFactory(); + } + + /** + * TODO(sergiitk): javadoc. + */ + public static class ClientTransportFactoryBuilderImpl implements ClientTransportFactoryBuilder { + @Override + public ClientTransportFactory buildClientTransportFactory() { + throw new UnsupportedOperationException(); + } + } + + /** + * An interface for Transport implementors to provide a default port to {@link + * io.grpc.NameResolver} for use in cases where the target string doesn't include a port. The + * default implementation returns {@link GrpcUtil#DEFAULT_PORT_SSL}. + */ + public interface ChannelBuilderDefaultPortProvider { + int getDefaultPort(); + } + + /** + * Default implementation of {@link ChannelBuilderDefaultPortProvider} that returns a fixed port. + */ + public static final class FixedPortProvider implements ChannelBuilderDefaultPortProvider { + private final int port; + + public FixedPortProvider(int port) { + this.port = port; + } + + @Override + public int getDefaultPort() { + return port; + } + } + + private static final class ManagedChannelDefaultPortProvider implements + ChannelBuilderDefaultPortProvider { + @Override + public int getDefaultPort() { + return GrpcUtil.DEFAULT_PORT_SSL; + } + } + private static class DirectAddressNameResolverFactory extends NameResolver.Factory { final SocketAddress address; final String authority; From ef6a366563bcb9b552cf4fbaa1ae914069bca6f6 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 15:32:05 -0400 Subject: [PATCH 05/11] AbstractManagedChannelImplBuilder: delete --- .../AbstractManagedChannelImplBuilder.java | 35 ------------------- .../internal/ManagedChannelImplBuilder.java | 2 +- 2 files changed, 1 insertion(+), 36 deletions(-) delete mode 100644 core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java deleted file mode 100644 index d78b1e80970..00000000000 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright 2014 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc.internal; - -import io.grpc.ManagedChannelBuilder; - -/** - * Abstract base class for channel builders. - * - * @param The concrete type of this builder. - */ -public abstract class AbstractManagedChannelImplBuilder - > extends ManagedChannelBuilder { - public static ManagedChannelBuilder forAddress(String name, int port) { - throw new UnsupportedOperationException("Subclass failed to hide static factory"); - } - - public static ManagedChannelBuilder forTarget(String target) { - throw new UnsupportedOperationException("Subclass failed to hide static factory"); - } -} diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index bfc18dfa5b0..b577f667076 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -52,7 +52,7 @@ * Default managed channel builder, for usage in Transport implementations. */ public final class ManagedChannelImplBuilder - extends AbstractManagedChannelImplBuilder { + extends ManagedChannelBuilder { private static final Logger log = Logger.getLogger(ManagedChannelImplBuilder.class.getName()); private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; private static final long DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES = 1L << 24; // 16M From eda8072c24a6893331f70afd83749a64b2e565d8 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 15:55:54 -0400 Subject: [PATCH 06/11] Merge tests --- .../ManagedChannelImplBuilderTest.java | 435 ++++++++++++++++ .../ManagedChannelImplBuilderTest2.java | 470 ------------------ 2 files changed, 435 insertions(+), 470 deletions(-) delete mode 100644 core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index 5d3f4cf4f14..e96f8a1ae1d 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -16,14 +16,40 @@ package io.grpc.internal; +import static com.google.common.truth.Truth.assertThat; +import static junit.framework.TestCase.assertFalse; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.google.common.util.concurrent.MoreExecutors; +import io.grpc.CallOptions; +import io.grpc.Channel; +import io.grpc.ClientCall; +import io.grpc.ClientInterceptor; +import io.grpc.CompressorRegistry; +import io.grpc.DecompressorRegistry; +import io.grpc.MethodDescriptor; +import io.grpc.NameResolver; import io.grpc.internal.ManagedChannelImplBuilder.ChannelBuilderDefaultPortProvider; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; import io.grpc.internal.ManagedChannelImplBuilder.FixedPortProvider; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.net.URI; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -41,6 +67,14 @@ public class ManagedChannelImplBuilderTest { private static final String DUMMY_TARGET = "fake-target"; private static final String DUMMY_AUTHORITY_VALID = "valid:1234"; private static final String DUMMY_AUTHORITY_INVALID = "[ : : 1]"; + private static final ClientInterceptor DUMMY_USER_INTERCEPTOR = + new ClientInterceptor() { + @Override + public ClientCall interceptCall( + MethodDescriptor method, CallOptions callOptions, Channel next) { + return next.newCall(method, callOptions); + } + }; @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @Rule public final ExpectedException thrown = ExpectedException.none(); @@ -48,6 +82,7 @@ public class ManagedChannelImplBuilderTest { @Mock private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder; @Mock private ChannelBuilderDefaultPortProvider mockChannelBuilderDefaultPortProvider; private ManagedChannelImplBuilder builder; + private ManagedChannelImplBuilder directAddressBuilder; @Before public void setUp() throws Exception { @@ -55,6 +90,11 @@ public void setUp() throws Exception { DUMMY_TARGET, mockClientTransportFactoryBuilder, mockChannelBuilderDefaultPortProvider); + directAddressBuilder = new ManagedChannelImplBuilder( + new SocketAddress() {}, + DUMMY_TARGET, + mockClientTransportFactoryBuilder, + mockChannelBuilderDefaultPortProvider); } /** Ensure buildTransportFactory() delegates to the custom implementation. */ @@ -93,6 +133,203 @@ public void getDefaultPort_fixedPortProvider() { assertEquals(DUMMY_PORT, builderFixedPortProvider.getDefaultPort()); } + @Test + public void executor_default() { + assertNotNull(builder.executorPool); + } + + @Test + public void executor_normal() { + Executor executor = mock(Executor.class); + assertEquals(builder, builder.executor(executor)); + assertEquals(executor, builder.executorPool.getObject()); + } + + @Test + public void executor_null() { + ObjectPool defaultValue = builder.executorPool; + builder.executor(mock(Executor.class)); + assertEquals(builder, builder.executor(null)); + assertEquals(defaultValue, builder.executorPool); + } + + @Test + public void directExecutor() { + assertEquals(builder, builder.directExecutor()); + assertEquals(MoreExecutors.directExecutor(), builder.executorPool.getObject()); + } + + @Test + public void offloadExecutor_normal() { + Executor executor = mock(Executor.class); + assertEquals(builder, builder.offloadExecutor(executor)); + assertEquals(executor, builder.offloadExecutorPool.getObject()); + } + + @Test + public void offloadExecutor_null() { + ObjectPool defaultValue = builder.offloadExecutorPool; + builder.offloadExecutor(mock(Executor.class)); + assertEquals(builder, builder.offloadExecutor(null)); + assertEquals(defaultValue, builder.offloadExecutorPool); + } + + @Test + public void nameResolverFactory_default() { + assertNotNull(builder.getNameResolverFactory()); + } + + @Test + @SuppressWarnings("deprecation") + public void nameResolverFactory_normal() { + NameResolver.Factory nameResolverFactory = mock(NameResolver.Factory.class); + assertEquals(builder, builder.nameResolverFactory(nameResolverFactory)); + assertEquals(nameResolverFactory, builder.getNameResolverFactory()); + } + + @Test + @SuppressWarnings("deprecation") + public void nameResolverFactory_null() { + NameResolver.Factory defaultValue = builder.getNameResolverFactory(); + builder.nameResolverFactory(mock(NameResolver.Factory.class)); + assertEquals(builder, builder.nameResolverFactory(null)); + assertEquals(defaultValue, builder.getNameResolverFactory()); + } + + @Test(expected = IllegalStateException.class) + @SuppressWarnings("deprecation") + public void nameResolverFactory_notAllowedWithDirectAddress() { + directAddressBuilder.nameResolverFactory(mock(NameResolver.Factory.class)); + } + + @Test + public void defaultLoadBalancingPolicy_default() { + assertEquals("pick_first", builder.defaultLbPolicy); + } + + @Test + public void defaultLoadBalancingPolicy_normal() { + assertEquals(builder, builder.defaultLoadBalancingPolicy("magic_balancer")); + assertEquals("magic_balancer", builder.defaultLbPolicy); + } + + @Test(expected = IllegalArgumentException.class) + public void defaultLoadBalancingPolicy_null() { + builder.defaultLoadBalancingPolicy(null); + } + + @Test(expected = IllegalStateException.class) + public void defaultLoadBalancingPolicy_notAllowedWithDirectAddress() { + directAddressBuilder.defaultLoadBalancingPolicy("magic_balancer"); + } + + @Test + public void fullStreamDecompression_default() { + assertFalse(builder.fullStreamDecompression); + } + + @Test + public void fullStreamDecompression_enabled() { + assertEquals(builder, builder.enableFullStreamDecompression()); + assertTrue(builder.fullStreamDecompression); + } + + @Test + public void decompressorRegistry_default() { + assertNotNull(builder.decompressorRegistry); + } + + @Test + public void decompressorRegistry_normal() { + DecompressorRegistry decompressorRegistry = DecompressorRegistry.emptyInstance(); + assertNotEquals(decompressorRegistry, builder.decompressorRegistry); + assertEquals(builder, builder.decompressorRegistry(decompressorRegistry)); + assertEquals(decompressorRegistry, builder.decompressorRegistry); + } + + @Test + public void decompressorRegistry_null() { + DecompressorRegistry defaultValue = builder.decompressorRegistry; + assertEquals(builder, builder.decompressorRegistry(DecompressorRegistry.emptyInstance())); + assertNotEquals(defaultValue, builder.decompressorRegistry); + builder.decompressorRegistry(null); + assertEquals(defaultValue, builder.decompressorRegistry); + } + + @Test + public void compressorRegistry_default() { + assertNotNull(builder.compressorRegistry); + } + + @Test + public void compressorRegistry_normal() { + CompressorRegistry compressorRegistry = CompressorRegistry.newEmptyInstance(); + assertNotEquals(compressorRegistry, builder.compressorRegistry); + assertEquals(builder, builder.compressorRegistry(compressorRegistry)); + assertEquals(compressorRegistry, builder.compressorRegistry); + } + + @Test + public void compressorRegistry_null() { + CompressorRegistry defaultValue = builder.compressorRegistry; + builder.compressorRegistry(CompressorRegistry.newEmptyInstance()); + assertNotEquals(defaultValue, builder.compressorRegistry); + assertEquals(builder, builder.compressorRegistry(null)); + assertEquals(defaultValue, builder.compressorRegistry); + } + + @Test + public void userAgent_default() { + assertNull(builder.userAgent); + } + + @Test + public void userAgent_normal() { + String userAgent = "user-agent/1"; + assertEquals(builder, builder.userAgent(userAgent)); + assertEquals(userAgent, builder.userAgent); + } + + @Test + public void userAgent_null() { + assertEquals(builder, builder.userAgent(null)); + assertNull(builder.userAgent); + + builder.userAgent("user-agent/1"); + builder.userAgent(null); + assertNull(builder.userAgent); + } + + @Test + public void overrideAuthority_default() { + assertNull(builder.authorityOverride); + } + + @Test + public void overrideAuthority_normal() { + String overrideAuthority = "best-authority"; + assertEquals(builder, builder.overrideAuthority(overrideAuthority)); + assertEquals(overrideAuthority, builder.authorityOverride); + } + + @Test(expected = NullPointerException.class) + public void overrideAuthority_null() { + builder.overrideAuthority(null); + } + + @Test(expected = IllegalArgumentException.class) + public void overrideAuthority_invalid() { + builder.overrideAuthority("not_allowed"); + } + + @Test + public void overrideAuthority_getNameResolverFactory() { + assertNull(builder.authorityOverride); + assertFalse(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); + builder.overrideAuthority("google.com"); + assertTrue(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); + } + @Test public void checkAuthority_validAuthorityAllowed() { assertEquals(DUMMY_AUTHORITY_VALID, builder.checkAuthority(DUMMY_AUTHORITY_VALID)); @@ -132,4 +369,202 @@ public void disableCheckAuthority_invalidAuthorityFailed() { builder.disableCheckAuthority().enableCheckAuthority(); builder.checkAuthority(DUMMY_AUTHORITY_INVALID); } + + @Test + public void makeTargetStringForDirectAddress_scopedIpv6() throws Exception { + InetSocketAddress address = new InetSocketAddress("0:0:0:0:0:0:0:0%0", 10005); + assertEquals("/0:0:0:0:0:0:0:0%0:10005", address.toString()); + String target = ManagedChannelImplBuilder.makeTargetStringForDirectAddress(address); + URI uri = new URI(target); + assertEquals("directaddress:////0:0:0:0:0:0:0:0%250:10005", target); + assertEquals(target, uri.toString()); + } + + @Test + public void getEffectiveInterceptors_default() { + builder.intercept(DUMMY_USER_INTERCEPTOR); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertEquals(3, effectiveInterceptors.size()); + assertThat(effectiveInterceptors.get(0).getClass().getName()) + .isEqualTo("io.grpc.census.CensusTracingModule$TracingClientInterceptor"); + assertThat(effectiveInterceptors.get(1).getClass().getName()) + .isEqualTo("io.grpc.census.CensusStatsModule$StatsClientInterceptor"); + assertThat(effectiveInterceptors.get(2)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); + } + + @Test + public void getEffectiveInterceptors_disableStats() { + builder.intercept(DUMMY_USER_INTERCEPTOR); + builder.setStatsEnabled(false); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertEquals(2, effectiveInterceptors.size()); + assertThat(effectiveInterceptors.get(0).getClass().getName()) + .isEqualTo("io.grpc.census.CensusTracingModule$TracingClientInterceptor"); + assertThat(effectiveInterceptors.get(1)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); + } + + @Test + public void getEffectiveInterceptors_disableTracing() { + builder.intercept(DUMMY_USER_INTERCEPTOR); + builder.setTracingEnabled(false); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertEquals(2, effectiveInterceptors.size()); + assertThat(effectiveInterceptors.get(0).getClass().getName()) + .isEqualTo("io.grpc.census.CensusStatsModule$StatsClientInterceptor"); + assertThat(effectiveInterceptors.get(1)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); + } + + @Test + public void getEffectiveInterceptors_disableBoth() { + builder.intercept(DUMMY_USER_INTERCEPTOR); + builder.setStatsEnabled(false); + builder.setTracingEnabled(false); + List effectiveInterceptors = builder.getEffectiveInterceptors(); + assertThat(effectiveInterceptors).containsExactly(DUMMY_USER_INTERCEPTOR); + } + + @Test + public void idleTimeout() { + assertEquals(ManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, + builder.getIdleTimeoutMillis()); + + builder.idleTimeout(Long.MAX_VALUE, TimeUnit.DAYS); + assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); + + builder.idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, + TimeUnit.DAYS); + assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); + + try { + builder.idleTimeout(0, TimeUnit.SECONDS); + fail("Should throw"); + } catch (IllegalArgumentException e) { + // expected + } + + builder.idleTimeout(1, TimeUnit.NANOSECONDS); + assertEquals(ManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, + builder.getIdleTimeoutMillis()); + + builder.idleTimeout(30, TimeUnit.SECONDS); + assertEquals(TimeUnit.SECONDS.toMillis(30), builder.getIdleTimeoutMillis()); + } + + @Test + public void maxRetryAttempts() { + assertEquals(5, builder.maxRetryAttempts); + + builder.maxRetryAttempts(3); + assertEquals(3, builder.maxRetryAttempts); + } + + @Test + public void maxHedgedAttempts() { + assertEquals(5, builder.maxHedgedAttempts); + + builder.maxHedgedAttempts(3); + assertEquals(3, builder.maxHedgedAttempts); + } + + @Test + public void retryBufferSize() { + assertEquals(1L << 24, builder.retryBufferSize); + + builder.retryBufferSize(3456L); + assertEquals(3456L, builder.retryBufferSize); + } + + @Test + public void perRpcBufferLimit() { + assertEquals(1L << 20, builder.perRpcBufferLimit); + + builder.perRpcBufferLimit(3456L); + assertEquals(3456L, builder.perRpcBufferLimit); + } + + @Test + public void retryBufferSizeInvalidArg() { + thrown.expect(IllegalArgumentException.class); + builder.retryBufferSize(0L); + } + + @Test + public void perRpcBufferLimitInvalidArg() { + thrown.expect(IllegalArgumentException.class); + builder.perRpcBufferLimit(0L); + } + + @Test + public void disableRetry() { + builder.enableRetry(); + assertTrue(builder.retryEnabled); + + builder.disableRetry(); + assertFalse(builder.retryEnabled); + + builder.enableRetry(); + assertTrue(builder.retryEnabled); + + builder.disableRetry(); + assertFalse(builder.retryEnabled); + } + + @Test + public void defaultServiceConfig_nullKey() { + Map config = new HashMap<>(); + config.put(null, "val"); + + thrown.expect(IllegalArgumentException.class); + builder.defaultServiceConfig(config); + } + + @Test + public void defaultServiceConfig_intKey() { + Map subConfig = new HashMap<>(); + subConfig.put(3, "val"); + Map config = new HashMap<>(); + config.put("key", subConfig); + + thrown.expect(IllegalArgumentException.class); + builder.defaultServiceConfig(config); + } + + @Test + public void defaultServiceConfig_intValue() { + Map config = new HashMap<>(); + config.put("key", 3); + + thrown.expect(IllegalArgumentException.class); + builder.defaultServiceConfig(config); + } + + @Test + public void defaultServiceConfig_nested() { + Map config = new HashMap<>(); + List list1 = new ArrayList<>(); + list1.add(123D); + list1.add(null); + list1.add(true); + list1.add("str"); + Map map2 = new HashMap<>(); + map2.put("key2", false); + map2.put("key3", null); + map2.put("key4", Collections.singletonList("v4")); + map2.put("key4", 3.14D); + map2.put("key5", new HashMap()); + list1.add(map2); + config.put("key1", list1); + + builder.defaultServiceConfig(config); + + assertThat(builder.defaultServiceConfig).containsExactlyEntriesIn(config); + } + + @Test + public void disableNameResolverServiceConfig() { + assertThat(builder.lookUpServiceConfig).isTrue(); + + builder.disableServiceConfigLookUp(); + assertThat(builder.lookUpServiceConfig).isFalse(); + } } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java deleted file mode 100644 index 8e6082c943b..00000000000 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest2.java +++ /dev/null @@ -1,470 +0,0 @@ -/* - * Copyright 2016 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc.internal; - -import static com.google.common.truth.Truth.assertThat; -import static junit.framework.TestCase.assertFalse; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import static org.mockito.Mockito.mock; - -import com.google.common.util.concurrent.MoreExecutors; -import io.grpc.CallOptions; -import io.grpc.Channel; -import io.grpc.ClientCall; -import io.grpc.ClientInterceptor; -import io.grpc.CompressorRegistry; -import io.grpc.DecompressorRegistry; -import io.grpc.MethodDescriptor; -import io.grpc.NameResolver; -import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; -import java.net.InetSocketAddress; -import java.net.SocketAddress; -import java.net.URI; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.concurrent.Executor; -import java.util.concurrent.TimeUnit; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.ExpectedException; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -/** Unit tests for {@link ManagedChannelImplBuilder}. */ -@RunWith(JUnit4.class) -public class ManagedChannelImplBuilderTest2 { - - @Rule - public final ExpectedException thrown = ExpectedException.none(); - - private static final ClientInterceptor DUMMY_USER_INTERCEPTOR = - new ClientInterceptor() { - @Override - public ClientCall interceptCall( - MethodDescriptor method, CallOptions callOptions, Channel next) { - return next.newCall(method, callOptions); - } - }; - - private final ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake", - new ClientTransportFactoryBuilderImpl(), null); - private final ManagedChannelImplBuilder directAddressBuilder = new ManagedChannelImplBuilder( - new SocketAddress() {}, "fake", new ClientTransportFactoryBuilderImpl(), null); - - @Test - public void executor_default() { - assertNotNull(builder.executorPool); - } - - @Test - public void executor_normal() { - Executor executor = mock(Executor.class); - assertEquals(builder, builder.executor(executor)); - assertEquals(executor, builder.executorPool.getObject()); - } - - @Test - public void executor_null() { - ObjectPool defaultValue = builder.executorPool; - builder.executor(mock(Executor.class)); - assertEquals(builder, builder.executor(null)); - assertEquals(defaultValue, builder.executorPool); - } - - @Test - public void directExecutor() { - assertEquals(builder, builder.directExecutor()); - assertEquals(MoreExecutors.directExecutor(), builder.executorPool.getObject()); - } - - @Test - public void offloadExecutor_normal() { - Executor executor = mock(Executor.class); - assertEquals(builder, builder.offloadExecutor(executor)); - assertEquals(executor, builder.offloadExecutorPool.getObject()); - } - - @Test - public void offloadExecutor_null() { - ObjectPool defaultValue = builder.offloadExecutorPool; - builder.offloadExecutor(mock(Executor.class)); - assertEquals(builder, builder.offloadExecutor(null)); - assertEquals(defaultValue, builder.offloadExecutorPool); - } - - @Test - public void nameResolverFactory_default() { - assertNotNull(builder.getNameResolverFactory()); - } - - @Test - @SuppressWarnings("deprecation") - public void nameResolverFactory_normal() { - NameResolver.Factory nameResolverFactory = mock(NameResolver.Factory.class); - assertEquals(builder, builder.nameResolverFactory(nameResolverFactory)); - assertEquals(nameResolverFactory, builder.getNameResolverFactory()); - } - - @Test - @SuppressWarnings("deprecation") - public void nameResolverFactory_null() { - NameResolver.Factory defaultValue = builder.getNameResolverFactory(); - builder.nameResolverFactory(mock(NameResolver.Factory.class)); - assertEquals(builder, builder.nameResolverFactory(null)); - assertEquals(defaultValue, builder.getNameResolverFactory()); - } - - @Test(expected = IllegalStateException.class) - @SuppressWarnings("deprecation") - public void nameResolverFactory_notAllowedWithDirectAddress() { - directAddressBuilder.nameResolverFactory(mock(NameResolver.Factory.class)); - } - - @Test - public void defaultLoadBalancingPolicy_default() { - assertEquals("pick_first", builder.defaultLbPolicy); - } - - @Test - public void defaultLoadBalancingPolicy_normal() { - assertEquals(builder, builder.defaultLoadBalancingPolicy("magic_balancer")); - assertEquals("magic_balancer", builder.defaultLbPolicy); - } - - @Test(expected = IllegalArgumentException.class) - public void defaultLoadBalancingPolicy_null() { - builder.defaultLoadBalancingPolicy(null); - } - - @Test(expected = IllegalStateException.class) - public void defaultLoadBalancingPolicy_notAllowedWithDirectAddress() { - directAddressBuilder.defaultLoadBalancingPolicy("magic_balancer"); - } - - @Test - public void fullStreamDecompression_default() { - assertFalse(builder.fullStreamDecompression); - } - - @Test - public void fullStreamDecompression_enabled() { - assertEquals(builder, builder.enableFullStreamDecompression()); - assertTrue(builder.fullStreamDecompression); - } - - @Test - public void decompressorRegistry_default() { - assertNotNull(builder.decompressorRegistry); - } - - @Test - public void decompressorRegistry_normal() { - DecompressorRegistry decompressorRegistry = DecompressorRegistry.emptyInstance(); - assertNotEquals(decompressorRegistry, builder.decompressorRegistry); - assertEquals(builder, builder.decompressorRegistry(decompressorRegistry)); - assertEquals(decompressorRegistry, builder.decompressorRegistry); - } - - @Test - public void decompressorRegistry_null() { - DecompressorRegistry defaultValue = builder.decompressorRegistry; - assertEquals(builder, builder.decompressorRegistry(DecompressorRegistry.emptyInstance())); - assertNotEquals(defaultValue, builder.decompressorRegistry); - builder.decompressorRegistry(null); - assertEquals(defaultValue, builder.decompressorRegistry); - } - - @Test - public void compressorRegistry_default() { - assertNotNull(builder.compressorRegistry); - } - - @Test - public void compressorRegistry_normal() { - CompressorRegistry compressorRegistry = CompressorRegistry.newEmptyInstance(); - assertNotEquals(compressorRegistry, builder.compressorRegistry); - assertEquals(builder, builder.compressorRegistry(compressorRegistry)); - assertEquals(compressorRegistry, builder.compressorRegistry); - } - - @Test - public void compressorRegistry_null() { - CompressorRegistry defaultValue = builder.compressorRegistry; - builder.compressorRegistry(CompressorRegistry.newEmptyInstance()); - assertNotEquals(defaultValue, builder.compressorRegistry); - assertEquals(builder, builder.compressorRegistry(null)); - assertEquals(defaultValue, builder.compressorRegistry); - } - - @Test - public void userAgent_default() { - assertNull(builder.userAgent); - } - - @Test - public void userAgent_normal() { - String userAgent = "user-agent/1"; - assertEquals(builder, builder.userAgent(userAgent)); - assertEquals(userAgent, builder.userAgent); - } - - @Test - public void userAgent_null() { - assertEquals(builder, builder.userAgent(null)); - assertNull(builder.userAgent); - - builder.userAgent("user-agent/1"); - builder.userAgent(null); - assertNull(builder.userAgent); - } - - @Test - public void overrideAuthority_default() { - assertNull(builder.authorityOverride); - } - - @Test - public void overrideAuthority_normal() { - String overrideAuthority = "best-authority"; - assertEquals(builder, builder.overrideAuthority(overrideAuthority)); - assertEquals(overrideAuthority, builder.authorityOverride); - } - - @Test(expected = NullPointerException.class) - public void overrideAuthority_null() { - builder.overrideAuthority(null); - } - - @Test(expected = IllegalArgumentException.class) - public void overrideAuthority_invalid() { - builder.overrideAuthority("not_allowed"); - } - - @Test - public void overrideAuthority_getNameResolverFactory() { - assertNull(builder.authorityOverride); - assertFalse(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); - builder.overrideAuthority("google.com"); - assertTrue(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); - } - - @Test - public void makeTargetStringForDirectAddress_scopedIpv6() throws Exception { - InetSocketAddress address = new InetSocketAddress("0:0:0:0:0:0:0:0%0", 10005); - assertEquals("/0:0:0:0:0:0:0:0%0:10005", address.toString()); - String target = ManagedChannelImplBuilder.makeTargetStringForDirectAddress(address); - URI uri = new URI(target); - assertEquals("directaddress:////0:0:0:0:0:0:0:0%250:10005", target); - assertEquals(target, uri.toString()); - } - - @Test - public void getEffectiveInterceptors_default() { - builder.intercept(DUMMY_USER_INTERCEPTOR); - List effectiveInterceptors = builder.getEffectiveInterceptors(); - assertEquals(3, effectiveInterceptors.size()); - assertThat(effectiveInterceptors.get(0).getClass().getName()) - .isEqualTo("io.grpc.census.CensusTracingModule$TracingClientInterceptor"); - assertThat(effectiveInterceptors.get(1).getClass().getName()) - .isEqualTo("io.grpc.census.CensusStatsModule$StatsClientInterceptor"); - assertThat(effectiveInterceptors.get(2)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); - } - - @Test - public void getEffectiveInterceptors_disableStats() { - builder.intercept(DUMMY_USER_INTERCEPTOR); - builder.setStatsEnabled(false); - List effectiveInterceptors = builder.getEffectiveInterceptors(); - assertEquals(2, effectiveInterceptors.size()); - assertThat(effectiveInterceptors.get(0).getClass().getName()) - .isEqualTo("io.grpc.census.CensusTracingModule$TracingClientInterceptor"); - assertThat(effectiveInterceptors.get(1)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); - } - - @Test - public void getEffectiveInterceptors_disableTracing() { - builder.intercept(DUMMY_USER_INTERCEPTOR); - builder.setTracingEnabled(false); - List effectiveInterceptors = builder.getEffectiveInterceptors(); - assertEquals(2, effectiveInterceptors.size()); - assertThat(effectiveInterceptors.get(0).getClass().getName()) - .isEqualTo("io.grpc.census.CensusStatsModule$StatsClientInterceptor"); - assertThat(effectiveInterceptors.get(1)).isSameInstanceAs(DUMMY_USER_INTERCEPTOR); - } - - @Test - public void getEffectiveInterceptors_disableBoth() { - builder.intercept(DUMMY_USER_INTERCEPTOR); - builder.setStatsEnabled(false); - builder.setTracingEnabled(false); - List effectiveInterceptors = builder.getEffectiveInterceptors(); - assertThat(effectiveInterceptors).containsExactly(DUMMY_USER_INTERCEPTOR); - } - - @Test - public void idleTimeout() { - assertEquals(ManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, - builder.getIdleTimeoutMillis()); - - builder.idleTimeout(Long.MAX_VALUE, TimeUnit.DAYS); - assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); - - builder.idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, - TimeUnit.DAYS); - assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); - - try { - builder.idleTimeout(0, TimeUnit.SECONDS); - fail("Should throw"); - } catch (IllegalArgumentException e) { - // expected - } - - builder.idleTimeout(1, TimeUnit.NANOSECONDS); - assertEquals(ManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, - builder.getIdleTimeoutMillis()); - - builder.idleTimeout(30, TimeUnit.SECONDS); - assertEquals(TimeUnit.SECONDS.toMillis(30), builder.getIdleTimeoutMillis()); - } - - @Test - public void maxRetryAttempts() { - assertEquals(5, builder.maxRetryAttempts); - - builder.maxRetryAttempts(3); - assertEquals(3, builder.maxRetryAttempts); - } - - @Test - public void maxHedgedAttempts() { - assertEquals(5, builder.maxHedgedAttempts); - - builder.maxHedgedAttempts(3); - assertEquals(3, builder.maxHedgedAttempts); - } - - @Test - public void retryBufferSize() { - assertEquals(1L << 24, builder.retryBufferSize); - - builder.retryBufferSize(3456L); - assertEquals(3456L, builder.retryBufferSize); - } - - @Test - public void perRpcBufferLimit() { - assertEquals(1L << 20, builder.perRpcBufferLimit); - - builder.perRpcBufferLimit(3456L); - assertEquals(3456L, builder.perRpcBufferLimit); - } - - @Test - public void retryBufferSizeInvalidArg() { - thrown.expect(IllegalArgumentException.class); - builder.retryBufferSize(0L); - } - - @Test - public void perRpcBufferLimitInvalidArg() { - thrown.expect(IllegalArgumentException.class); - builder.perRpcBufferLimit(0L); - } - - @Test - public void disableRetry() { - builder.enableRetry(); - assertTrue(builder.retryEnabled); - - builder.disableRetry(); - assertFalse(builder.retryEnabled); - - builder.enableRetry(); - assertTrue(builder.retryEnabled); - - builder.disableRetry(); - assertFalse(builder.retryEnabled); - } - - @Test - public void defaultServiceConfig_nullKey() { - Map config = new HashMap<>(); - config.put(null, "val"); - - thrown.expect(IllegalArgumentException.class); - builder.defaultServiceConfig(config); - } - - @Test - public void defaultServiceConfig_intKey() { - Map subConfig = new HashMap<>(); - subConfig.put(3, "val"); - Map config = new HashMap<>(); - config.put("key", subConfig); - - thrown.expect(IllegalArgumentException.class); - builder.defaultServiceConfig(config); - } - - @Test - public void defaultServiceConfig_intValue() { - Map config = new HashMap<>(); - config.put("key", 3); - - thrown.expect(IllegalArgumentException.class); - builder.defaultServiceConfig(config); - } - - @Test - public void defaultServiceConfig_nested() { - Map config = new HashMap<>(); - List list1 = new ArrayList<>(); - list1.add(123D); - list1.add(null); - list1.add(true); - list1.add("str"); - Map map2 = new HashMap<>(); - map2.put("key2", false); - map2.put("key3", null); - map2.put("key4", Collections.singletonList("v4")); - map2.put("key4", 3.14D); - map2.put("key5", new HashMap()); - list1.add(map2); - config.put("key1", list1); - - builder.defaultServiceConfig(config); - - assertThat(builder.defaultServiceConfig).containsExactlyEntriesIn(config); - } - - @Test - public void disableNameResolverServiceConfig() { - assertThat(builder.lookUpServiceConfig).isTrue(); - - builder.disableServiceConfigLookUp(); - assertThat(builder.lookUpServiceConfig).isFalse(); - } -} From acf75cf1e71e9ed8631114d38a727e0105d05c9e Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 16:49:34 -0400 Subject: [PATCH 07/11] Order package-private methonds --- .../internal/ManagedChannelImplBuilder.java | 171 +++++++++--------- 1 file changed, 86 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index b577f667076..f8b6a9ba8b3 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -189,6 +189,7 @@ static String makeTargetStringForDirectAddress(SocketAddress address) { * ClientTransportFactory} appropriate the channel. This method is meant for Transport * implementors and should not be used by normal users. */ + @VisibleForTesting ClientTransportFactory buildTransportFactory() { return clientTransportFactoryBuilder.buildClientTransportFactory(); } @@ -249,6 +250,75 @@ public ManagedChannelImplBuilder intercept(ClientInterceptor... interceptors) { return intercept(Arrays.asList(interceptors)); } + // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know + // what should be the desired behavior for retry + stats/tracing. + // TODO(zdapeng): FIX IT + @VisibleForTesting + List getEffectiveInterceptors() { + List effectiveInterceptors = + new ArrayList<>(this.interceptors); + temporarilyDisableRetry = false; + if (statsEnabled) { + temporarilyDisableRetry = true; + ClientInterceptor statsInterceptor = null; + try { + Class censusStatsAccessor = + Class.forName("io.grpc.census.InternalCensusStatsAccessor"); + Method getClientInterceptorMethod = + censusStatsAccessor.getDeclaredMethod( + "getClientInterceptor", + boolean.class, + boolean.class, + boolean.class); + statsInterceptor = + (ClientInterceptor) getClientInterceptorMethod + .invoke( + null, + recordStartedRpcs, + recordFinishedRpcs, + recordRealTimeMetrics); + } catch (ClassNotFoundException e) { + // Replace these separate catch statements with multicatch when Android min-API >= 19 + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (NoSuchMethodException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (IllegalAccessException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (InvocationTargetException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } + if (statsInterceptor != null) { + // First interceptor runs last (see ClientInterceptors.intercept()), so that no + // other interceptor can override the tracer factory we set in CallOptions. + effectiveInterceptors.add(0, statsInterceptor); + } + } + if (tracingEnabled) { + temporarilyDisableRetry = true; + ClientInterceptor tracingInterceptor = null; + try { + Class censusTracingAccessor = + Class.forName("io.grpc.census.InternalCensusTracingAccessor"); + Method getClientInterceptroMethod = + censusTracingAccessor.getDeclaredMethod("getClientInterceptor"); + tracingInterceptor = (ClientInterceptor) getClientInterceptroMethod.invoke(null); + } catch (ClassNotFoundException e) { + // Replace these separate catch statements with multicatch when Android min-API >= 19 + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (NoSuchMethodException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (IllegalAccessException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } catch (InvocationTargetException e) { + log.log(Level.FINE, "Unable to apply census stats", e); + } + if (tracingInterceptor != null) { + effectiveInterceptors.add(0, tracingInterceptor); + } + } + return effectiveInterceptors; + } + @Override public ManagedChannelImplBuilder userAgent(@Nullable String userAgent) { this.userAgent = userAgent; @@ -275,6 +345,17 @@ public ManagedChannelImplBuilder nameResolverFactory(NameResolver.Factory resolv return this; } + /** + * Returns a {@link NameResolver.Factory} for the channel. + */ + NameResolver.Factory getNameResolverFactory() { + if (authorityOverride == null) { + return nameResolverFactory; + } else { + return new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); + } + } + @Override public ManagedChannelImplBuilder defaultLoadBalancingPolicy(String policy) { Preconditions.checkState(directServerAddress == null, @@ -324,6 +405,11 @@ public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { return this; } + @VisibleForTesting + long getIdleTimeoutMillis() { + return idleTimeoutMillis; + } + @Override public ManagedChannelImplBuilder maxRetryAttempts(int maxRetryAttempts) { this.maxRetryAttempts = maxRetryAttempts; @@ -511,11 +597,6 @@ public ManagedChannelImplBuilder enableCheckAuthority() { return this; } - @VisibleForTesting - final long getIdleTimeoutMillis() { - return idleTimeoutMillis; - } - /** * Verifies the authority is valid. */ @@ -534,86 +615,6 @@ public ObjectPool getOffloadExecutorPool() { return this.offloadExecutorPool; } - /** - * Returns a {@link NameResolver.Factory} for the channel. - */ - NameResolver.Factory getNameResolverFactory() { - if (authorityOverride == null) { - return nameResolverFactory; - } else { - return new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); - } - } - - // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know - // what should be the desired behavior for retry + stats/tracing. - // TODO(zdapeng): FIX IT - @VisibleForTesting - final List getEffectiveInterceptors() { - List effectiveInterceptors = - new ArrayList<>(this.interceptors); - temporarilyDisableRetry = false; - if (statsEnabled) { - temporarilyDisableRetry = true; - ClientInterceptor statsInterceptor = null; - try { - Class censusStatsAccessor = - Class.forName("io.grpc.census.InternalCensusStatsAccessor"); - Method getClientInterceptorMethod = - censusStatsAccessor.getDeclaredMethod( - "getClientInterceptor", - boolean.class, - boolean.class, - boolean.class); - statsInterceptor = - (ClientInterceptor) getClientInterceptorMethod - .invoke( - null, - recordStartedRpcs, - recordFinishedRpcs, - recordRealTimeMetrics); - } catch (ClassNotFoundException e) { - // Replace these separate catch statements with multicatch when Android min-API >= 19 - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (NoSuchMethodException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (IllegalAccessException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (InvocationTargetException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } - if (statsInterceptor != null) { - // First interceptor runs last (see ClientInterceptors.intercept()), so that no - // other interceptor can override the tracer factory we set in CallOptions. - effectiveInterceptors.add(0, statsInterceptor); - } - } - if (tracingEnabled) { - temporarilyDisableRetry = true; - ClientInterceptor tracingInterceptor = null; - try { - Class censusTracingAccessor = - Class.forName("io.grpc.census.InternalCensusTracingAccessor"); - Method getClientInterceptroMethod = - censusTracingAccessor.getDeclaredMethod("getClientInterceptor"); - tracingInterceptor = (ClientInterceptor) getClientInterceptroMethod.invoke(null); - } catch (ClassNotFoundException e) { - // Replace these separate catch statements with multicatch when Android min-API >= 19 - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (NoSuchMethodException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (IllegalAccessException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } catch (InvocationTargetException e) { - log.log(Level.FINE, "Unable to apply census stats", e); - } - if (tracingInterceptor != null) { - effectiveInterceptors.add(0, tracingInterceptor); - } - } - return effectiveInterceptors; - } - public static ManagedChannelBuilder forAddress(String name, int port) { throw new UnsupportedOperationException( "ClientTransportFactoryBuilder is required, use a constructor"); From 5e0fd5b410fb1ce7ed51950e5bf25dcf373dd9e1 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 17:05:42 -0400 Subject: [PATCH 08/11] Order things + Make a getter for authorityOverride --- .../internal/ManagedChannelImplBuilder.java | 34 +++++++++---------- .../ManagedChannelImplBuilderTest.java | 6 ++-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index f8b6a9ba8b3..9851f5be984 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -77,15 +77,12 @@ public final class ManagedChannelImplBuilder static final long IDLE_MODE_DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(30); final String target; - @Nullable - private final SocketAddress directServerAddress; + @Nullable private final SocketAddress directServerAddress; private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; private final List interceptors = new ArrayList<>(); - @VisibleForTesting - @Nullable - String authorityOverride; + @Nullable private String authorityOverride; private boolean authorityCheckerDisabled; private boolean statsEnabled = true; private boolean recordStartedRpcs = true; @@ -108,25 +105,22 @@ public final class ManagedChannelImplBuilder long retryBufferSize = DEFAULT_RETRY_BUFFER_SIZE_IN_BYTES; long perRpcBufferLimit = DEFAULT_PER_RPC_BUFFER_LIMIT_IN_BYTES; boolean retryEnabled = false; // TODO(zdapeng): default to true - - long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS; - @Nullable - String userAgent; - String defaultLbPolicy = GrpcUtil.DEFAULT_LB_POLICY; - boolean fullStreamDecompression; // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know // what should be the desired behavior for retry + stats/tracing. // TODO(zdapeng): delete me boolean temporarilyDisableRetry; + + long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS; + @Nullable String userAgent; + String defaultLbPolicy = GrpcUtil.DEFAULT_LB_POLICY; + boolean fullStreamDecompression; + int maxTraceEvents; InternalChannelz channelz = InternalChannelz.instance(); - @Nullable - Map defaultServiceConfig; + @Nullable Map defaultServiceConfig; boolean lookUpServiceConfig = true; - @Nullable - BinaryLog binlog; - @Nullable - ProxyDetector proxyDetector; + @Nullable BinaryLog binlog; + @Nullable ProxyDetector proxyDetector; /** * Creates a new managed channel builder with a target string, which can be either a valid {@link @@ -331,6 +325,12 @@ public ManagedChannelImplBuilder overrideAuthority(String authority) { return this; } + @Nullable + @VisibleForTesting + String getOverrideAuthority() { + return authorityOverride; + } + @Deprecated @Override public ManagedChannelImplBuilder nameResolverFactory(NameResolver.Factory resolverFactory) { diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index e96f8a1ae1d..c92d2965ceb 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -302,14 +302,14 @@ public void userAgent_null() { @Test public void overrideAuthority_default() { - assertNull(builder.authorityOverride); + assertNull(builder.getOverrideAuthority()); } @Test public void overrideAuthority_normal() { String overrideAuthority = "best-authority"; assertEquals(builder, builder.overrideAuthority(overrideAuthority)); - assertEquals(overrideAuthority, builder.authorityOverride); + assertEquals(overrideAuthority, builder.getOverrideAuthority()); } @Test(expected = NullPointerException.class) @@ -324,7 +324,7 @@ public void overrideAuthority_invalid() { @Test public void overrideAuthority_getNameResolverFactory() { - assertNull(builder.authorityOverride); + assertNull(builder.getOverrideAuthority()); assertFalse(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); builder.overrideAuthority("google.com"); assertTrue(builder.getNameResolverFactory() instanceof OverrideAuthorityNameResolverFactory); From 0a768e62fe6912a71e0224b0b49424553a8fc421 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 18:05:22 -0400 Subject: [PATCH 09/11] Check the rest of the properties --- .../internal/ManagedChannelImplBuilder.java | 18 ++++++------------ .../ManagedChannelImplBuilderTest.java | 10 +++++----- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 9851f5be984..709bf306307 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -70,13 +70,13 @@ public final class ManagedChannelImplBuilder static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1); /** An idle timeout larger than this would disable idle mode. */ - @VisibleForTesting static final long IDLE_MODE_MAX_TIMEOUT_DAYS = 30; + @VisibleForTesting + static final long IDLE_MODE_MAX_TIMEOUT_DAYS = 30; /** The default idle timeout. */ @VisibleForTesting static final long IDLE_MODE_DEFAULT_TIMEOUT_MILLIS = TimeUnit.MINUTES.toMillis(30); - final String target; @Nullable private final SocketAddress directServerAddress; private final ClientTransportFactoryBuilder clientTransportFactoryBuilder; private final ChannelBuilderDefaultPortProvider channelBuilderDefaultPortProvider; @@ -90,6 +90,8 @@ public final class ManagedChannelImplBuilder private boolean recordRealTimeMetrics = false; private boolean tracingEnabled = true; + // Package-private properties below used to build ManagedChannelImpl. + final String target; ObjectPool executorPool = DEFAULT_EXECUTOR_POOL; ObjectPool offloadExecutorPool = DEFAULT_EXECUTOR_POOL; @@ -98,7 +100,7 @@ public final class ManagedChannelImplBuilder final NameResolverRegistry nameResolverRegistry = NameResolverRegistry.getDefaultRegistry(); // Access via getter, which may perform authority override as needed - NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); + private NameResolver.Factory nameResolverFactory = nameResolverRegistry.asFactory(); int maxRetryAttempts = 5; int maxHedgedAttempts = 5; @@ -107,14 +109,11 @@ public final class ManagedChannelImplBuilder boolean retryEnabled = false; // TODO(zdapeng): default to true // Temporarily disable retry when stats or tracing is enabled to avoid breakage, until we know // what should be the desired behavior for retry + stats/tracing. - // TODO(zdapeng): delete me - boolean temporarilyDisableRetry; - + boolean temporarilyDisableRetry; // TODO(zdapeng): delete me long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS; @Nullable String userAgent; String defaultLbPolicy = GrpcUtil.DEFAULT_LB_POLICY; boolean fullStreamDecompression; - int maxTraceEvents; InternalChannelz channelz = InternalChannelz.instance(); @Nullable Map defaultServiceConfig; @@ -405,11 +404,6 @@ public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { return this; } - @VisibleForTesting - long getIdleTimeoutMillis() { - return idleTimeoutMillis; - } - @Override public ManagedChannelImplBuilder maxRetryAttempts(int maxRetryAttempts) { this.maxRetryAttempts = maxRetryAttempts; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java index c92d2965ceb..27d066b3225 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java @@ -426,14 +426,14 @@ public void getEffectiveInterceptors_disableBoth() { @Test public void idleTimeout() { assertEquals(ManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS, - builder.getIdleTimeoutMillis()); + builder.idleTimeoutMillis); builder.idleTimeout(Long.MAX_VALUE, TimeUnit.DAYS); - assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); + assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.idleTimeoutMillis); builder.idleTimeout(ManagedChannelImplBuilder.IDLE_MODE_MAX_TIMEOUT_DAYS, TimeUnit.DAYS); - assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.getIdleTimeoutMillis()); + assertEquals(ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, builder.idleTimeoutMillis); try { builder.idleTimeout(0, TimeUnit.SECONDS); @@ -444,10 +444,10 @@ public void idleTimeout() { builder.idleTimeout(1, TimeUnit.NANOSECONDS); assertEquals(ManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS, - builder.getIdleTimeoutMillis()); + builder.idleTimeoutMillis); builder.idleTimeout(30, TimeUnit.SECONDS); - assertEquals(TimeUnit.SECONDS.toMillis(30), builder.getIdleTimeoutMillis()); + assertEquals(TimeUnit.SECONDS.toMillis(30), builder.idleTimeoutMillis); } @Test From 96cbae8e004727b7246113a4f7214c95d342e2ea Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 18:27:10 -0400 Subject: [PATCH 10/11] Convenience ClientTransportFactoryBuilderImpl() --- .../io/grpc/internal/ManagedChannelImplBuilder.java | 2 +- .../grpc/internal/ManagedChannelImplIdlenessTest.java | 9 ++------- .../java/io/grpc/internal/ManagedChannelImplTest.java | 10 +++------- .../grpc/internal/ServiceConfigErrorHandlingTest.java | 10 ++-------- 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 709bf306307..19fe30318c6 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -90,7 +90,7 @@ public final class ManagedChannelImplBuilder private boolean recordRealTimeMetrics = false; private boolean tracingEnabled = true; - // Package-private properties below used to build ManagedChannelImpl. + // Package-private properties below are accessed directly to build ManagedChannelImpl. final String target; ObjectPool executorPool = DEFAULT_EXECUTOR_POOL; ObjectPool offloadExecutorPool = DEFAULT_EXECUTOR_POOL; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index c551d26449c..093bfc2b369 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -62,7 +62,7 @@ import io.grpc.Status; import io.grpc.StringMarshaller; import io.grpc.internal.FakeClock.ScheduledTask; -import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; import io.grpc.internal.TestUtils.MockClientTransportInfo; import java.net.SocketAddress; import java.net.URI; @@ -161,12 +161,7 @@ public void setUp() { .thenReturn(timer.getScheduledExecutorService()); ManagedChannelImplBuilder builder = new ManagedChannelImplBuilder("fake://target", - new ClientTransportFactoryBuilder() { - @Override public ClientTransportFactory buildClientTransportFactory() { - throw new UnsupportedOperationException(); - } - }, - null); + new ClientTransportFactoryBuilderImpl(), null); builder .nameResolverFactory(mockNameResolverFactory) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index cd14f409ca3..24f80bd384a 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -108,6 +108,7 @@ import io.grpc.internal.InternalSubchannel.TransportLogger; import io.grpc.internal.ManagedChannelImpl.ScParser; import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; import io.grpc.internal.ManagedChannelImplBuilder.FixedPortProvider; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TestUtils.MockClientTransportInfo; @@ -328,13 +329,8 @@ public void setUp() throws Exception { .thenReturn(balancerRpcExecutor.getScheduledExecutorService()); channelBuilder = new ManagedChannelImplBuilder(TARGET, - new ClientTransportFactoryBuilder() { - @Override - public ClientTransportFactory buildClientTransportFactory() { - throw new UnsupportedOperationException(); - } - }, - new FixedPortProvider(DEFAULT_PORT)); + new ClientTransportFactoryBuilderImpl(), new FixedPortProvider(DEFAULT_PORT)); + channelBuilder .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) .defaultLoadBalancingPolicy(MOCK_POLICY_NAME) diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java index 49f094a6adb..df43e8dddd7 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java @@ -46,7 +46,7 @@ import io.grpc.NameResolver; import io.grpc.NameResolver.ConfigOrError; import io.grpc.Status; -import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilder; +import io.grpc.internal.ManagedChannelImplBuilder.ClientTransportFactoryBuilderImpl; import io.grpc.internal.ManagedChannelImplBuilder.FixedPortProvider; import java.net.SocketAddress; import java.net.URI; @@ -200,13 +200,7 @@ public void setUp() throws Exception { when(executorPool.getObject()).thenReturn(executor.getScheduledExecutorService()); channelBuilder = new ManagedChannelImplBuilder(TARGET, - new ClientTransportFactoryBuilder() { - @Override - public ClientTransportFactory buildClientTransportFactory() { - throw new UnsupportedOperationException(); - } - }, - new FixedPortProvider(DEFAULT_PORT)); + new ClientTransportFactoryBuilderImpl(), new FixedPortProvider(DEFAULT_PORT)); channelBuilder .nameResolverFactory(new FakeNameResolverFactory.Builder(expectedUri).build()) From 3b6df6a470f1486cd42594204f044359489465a1 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 14 Sep 2020 18:37:39 -0400 Subject: [PATCH 11/11] Remove superficial this --- .../internal/ManagedChannelImplBuilder.java | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java index 19fe30318c6..3aa2f5fda9e 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java @@ -215,9 +215,9 @@ public ManagedChannelImplBuilder directExecutor() { @Override public ManagedChannelImplBuilder executor(Executor executor) { if (executor != null) { - this.executorPool = new FixedObjectPool<>(executor); + executorPool = new FixedObjectPool<>(executor); } else { - this.executorPool = DEFAULT_EXECUTOR_POOL; + executorPool = DEFAULT_EXECUTOR_POOL; } return this; } @@ -225,9 +225,9 @@ public ManagedChannelImplBuilder executor(Executor executor) { @Override public ManagedChannelImplBuilder offloadExecutor(Executor executor) { if (executor != null) { - this.offloadExecutorPool = new FixedObjectPool<>(executor); + offloadExecutorPool = new FixedObjectPool<>(executor); } else { - this.offloadExecutorPool = DEFAULT_EXECUTOR_POOL; + offloadExecutorPool = DEFAULT_EXECUTOR_POOL; } return this; } @@ -320,7 +320,7 @@ public ManagedChannelImplBuilder userAgent(@Nullable String userAgent) { @Override public ManagedChannelImplBuilder overrideAuthority(String authority) { - this.authorityOverride = checkAuthority(authority); + authorityOverride = checkAuthority(authority); return this; } @@ -337,9 +337,9 @@ public ManagedChannelImplBuilder nameResolverFactory(NameResolver.Factory resolv "directServerAddress is set (%s), which forbids the use of NameResolverFactory", directServerAddress); if (resolverFactory != null) { - this.nameResolverFactory = resolverFactory; + nameResolverFactory = resolverFactory; } else { - this.nameResolverFactory = nameResolverRegistry.asFactory(); + nameResolverFactory = nameResolverRegistry.asFactory(); } return this; } @@ -361,22 +361,22 @@ public ManagedChannelImplBuilder defaultLoadBalancingPolicy(String policy) { "directServerAddress is set (%s), which forbids the use of load-balancing policy", directServerAddress); Preconditions.checkArgument(policy != null, "policy cannot be null"); - this.defaultLbPolicy = policy; + defaultLbPolicy = policy; return this; } @Override public ManagedChannelImplBuilder enableFullStreamDecompression() { - this.fullStreamDecompression = true; + fullStreamDecompression = true; return this; } @Override public ManagedChannelImplBuilder decompressorRegistry(DecompressorRegistry registry) { if (registry != null) { - this.decompressorRegistry = registry; + decompressorRegistry = registry; } else { - this.decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; + decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY; } return this; } @@ -384,9 +384,9 @@ public ManagedChannelImplBuilder decompressorRegistry(DecompressorRegistry regis @Override public ManagedChannelImplBuilder compressorRegistry(CompressorRegistry registry) { if (registry != null) { - this.compressorRegistry = registry; + compressorRegistry = registry; } else { - this.compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; + compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY; } return this; } @@ -397,9 +397,9 @@ public ManagedChannelImplBuilder idleTimeout(long value, TimeUnit unit) { // We convert to the largest unit to avoid overflow if (unit.toDays(value) >= IDLE_MODE_MAX_TIMEOUT_DAYS) { // This disables idle mode - this.idleTimeoutMillis = ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE; + idleTimeoutMillis = ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE; } else { - this.idleTimeoutMillis = Math.max(unit.toMillis(value), IDLE_MODE_MIN_TIMEOUT_MILLIS); + idleTimeoutMillis = Math.max(unit.toMillis(value), IDLE_MODE_MIN_TIMEOUT_MILLIS); } return this; } @@ -472,7 +472,7 @@ public ManagedChannelImplBuilder defaultServiceConfig(@Nullable Map s @Override public ManagedChannelImplBuilder disableServiceConfigLookUp() { - this.lookUpServiceConfig = false; + lookUpServiceConfig = false; return this; } @@ -606,7 +606,7 @@ String checkAuthority(String authority) { * Returns the internal offload executor pool for offloading tasks. */ public ObjectPool getOffloadExecutorPool() { - return this.offloadExecutorPool; + return offloadExecutorPool; } public static ManagedChannelBuilder forAddress(String name, int port) {