From 9388ee4786b6461d88cd173665a760ed01eda8c3 Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Mon, 7 Jan 2019 15:47:38 -0800 Subject: [PATCH 1/7] WIP: NameResolver.CreationParams --- core/src/main/java/io/grpc/NameResolver.java | 53 ++++++++++++++++++- .../java/io/grpc/NameResolverProvider.java | 3 +- .../AbstractManagedChannelImplBuilder.java | 7 ++- .../io/grpc/internal/DnsNameResolver.java | 10 +--- .../internal/DnsNameResolverProvider.java | 2 +- .../io/grpc/internal/ManagedChannelImpl.java | 4 +- .../OverrideAuthorityNameResolverFactory.java | 2 +- 7 files changed, 63 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 845dcb02fde..014809ab863 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -16,6 +16,7 @@ package io.grpc; +import com.google.common.base.MoreObjects; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -94,8 +95,12 @@ public abstract static class Factory { * The port number used in case the target or the underlying naming system doesn't provide a * port number. * + * @deprecated this will be deleted along with {@link #newNameResolver(URI, Attributes)} in + * a future release. + * * @since 1.0.0 */ + @Deprecated public static final Attributes.Key PARAMS_DEFAULT_PORT = Attributes.Key.create("params-default-port"); @@ -107,10 +112,39 @@ public abstract static class Factory { * @param targetUri the target URI to be resolved, whose scheme must not be {@code null} * @param params optional parameters. Canonical keys are defined as {@code PARAMS_*} fields in * {@link Factory}. + * + * @deprecated Implement {@link #newNameResolver(URI, Params)} instead. This is going to be + * deleted in a future release. + * * @since 1.0.0 */ @Nullable - public abstract NameResolver newNameResolver(URI targetUri, Attributes params); + @Deprecated + public NameResolver newNameResolver(URI targetUri, final Attributes params) { + return newNameResolver(targetUri, new CreationParams() { + @Override + public int getDefaultPort() { + return params.get(PARAMS_DEFAULT_PORT); + } + }); + } + + /** + * Creates a {@link NameResolver} for the given target URI, or {@code null} if the given URI + * cannot be resolved by this factory. The decision should be solely based on the scheme of the + * URI. + * + * @param targetUri the target URI to be resolved, whose scheme must not be {@code null} + * @param params additional parameters that may be used by the NameResolver implementation + * + * @since 1.19.0 + */ + @Nullable + public NameResolver newNameResolver(URI targetUri, CreationParams params) { + return newNameResolver( + targetUri, + Attributes.newBuilder().set(PARAMS_DEFAULT_PORT, params.getDefaultPort()).build()); + } /** * Returns the default scheme, which will be used to construct a URI when {@link @@ -162,4 +196,21 @@ void onAddresses( @Retention(RetentionPolicy.SOURCE) @Documented public @interface ResolutionResultAttr {} + + /** + * Parameters passed to {@link Factory#newNameResolver(URI, CreationParams)}. + */ + public static abstract class CreationParams { + /** + * The port number used in case the target or the underlying naming system doesn't provide a + * port number. + */ + public abstract int getDefaultPort(); + + public final String toString() { + return MoreObjects.toStringHelper(this) + .add("defaultPort", getDefaultPort()) + .toString(); + } + } } diff --git a/core/src/main/java/io/grpc/NameResolverProvider.java b/core/src/main/java/io/grpc/NameResolverProvider.java index f0b76ea70c2..f4137e09540 100644 --- a/core/src/main/java/io/grpc/NameResolverProvider.java +++ b/core/src/main/java/io/grpc/NameResolverProvider.java @@ -44,6 +44,7 @@ public abstract class NameResolverProvider extends NameResolver.Factory { * @since 1.0.0 */ @SuppressWarnings("unused") // Avoids outside callers accidentally depending on the super class. + @Deprecated public static final Attributes.Key PARAMS_DEFAULT_PORT = NameResolver.Factory.PARAMS_DEFAULT_PORT; @@ -106,7 +107,7 @@ private static final class NameResolverFactory extends NameResolver.Factory { @Override @Nullable - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.CreationParams params) { checkForProviders(); for (NameResolverProvider provider : providers) { NameResolver resolver = provider.newNameResolver(targetUri, params); diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 3830c075fd6..b2384207248 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -479,9 +479,8 @@ final List getEffectiveInterceptors() { protected abstract ClientTransportFactory buildTransportFactory(); /** - * Subclasses can override this method to provide additional parameters to {@link - * NameResolver.Factory#newNameResolver}. The default implementation returns {@link - * Attributes#EMPTY}. + * Subclasses can override this method to provide parameters to {@link + * NameResolver.Factory#newNameResolver}. The default implementation returns an */ protected Attributes getNameResolverParams() { return Attributes.EMPTY; @@ -508,7 +507,7 @@ private static class DirectAddressNameResolverFactory extends NameResolver.Facto } @Override - public NameResolver newNameResolver(URI notUsedUri, Attributes params) { + public NameResolver newNameResolver(URI notUsedUri, NameResolver.CreationParams params) { return new NameResolver() { @Override public String getServiceAuthority() { diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 2e0735f1099..ee48ac96e1e 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -147,7 +147,7 @@ final class DnsNameResolver extends NameResolver { private final Runnable resolveRunnable; - DnsNameResolver(@Nullable String nsAuthority, String name, Attributes params, + DnsNameResolver(@Nullable String nsAuthority, String name, CreationParams params, Resource executorResource, ProxyDetector proxyDetector, Stopwatch stopwatch, boolean isAndroid) { // TODO: if a DNS server is provided as nsAuthority, use it. @@ -161,13 +161,7 @@ final class DnsNameResolver extends NameResolver { "nameUri (%s) doesn't have an authority", nameUri); host = nameUri.getHost(); if (nameUri.getPort() == -1) { - Integer defaultPort = params.get(NameResolver.Factory.PARAMS_DEFAULT_PORT); - if (defaultPort != null) { - port = defaultPort; - } else { - throw new IllegalArgumentException( - "name '" + name + "' doesn't contain a port, and default port is not set in params"); - } + port = params.getDefaultPort(); } else { port = nameUri.getPort(); } diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index eda7754477c..f21eb00f7ae 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -43,7 +43,7 @@ public final class DnsNameResolverProvider extends NameResolverProvider { private static final String SCHEME = "dns"; @Override - public DnsNameResolver newNameResolver(URI targetUri, Attributes params) { + public DnsNameResolver newNameResolver(URI targetUri, CreationParams params) { if (SCHEME.equals(targetUri.getScheme())) { String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath"); Preconditions.checkArgument(targetPath.startsWith("/"), diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 94a485c8152..f8b74d30d12 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -125,7 +125,7 @@ final class ManagedChannelImpl extends ManagedChannel implements private final InternalLogId logId; private final String target; private final NameResolver.Factory nameResolverFactory; - private final Attributes nameResolverParams; + private final NameResolver.CreationParams nameResolverParams; private final LoadBalancer.Factory loadBalancerFactory; private final ClientTransportFactory transportFactory; private final ScheduledExecutorForBalancer scheduledExecutorForBalancer; @@ -605,7 +605,7 @@ public CallTracer create() { @VisibleForTesting static NameResolver getNameResolver(String target, NameResolver.Factory nameResolverFactory, - Attributes nameResolverParams) { + NameResolver.CreationParams nameResolverParams) { // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending // "dns:///". URI targetUri = null; diff --git a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java b/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java index 1e2154a4e08..17494135592 100644 --- a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java +++ b/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java @@ -43,7 +43,7 @@ final class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { @Nullable @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, CreationParams params) { final NameResolver resolver = delegate.newNameResolver(targetUri, params); // Do not wrap null values. We do not want to impede error signaling. if (resolver == null) { From 7faed8bea3186583a1f3f5f1ffe61860dfb494b6 Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Wed, 6 Feb 2019 16:49:52 -0800 Subject: [PATCH 2/7] WIP: add getProxyDetector() --- core/src/main/java/io/grpc/NameResolver.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 7e33155018f..6225177a7a3 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -106,8 +106,12 @@ public abstract static class Factory { /** * Proxy detector used in name resolution. + * + * @deprecated this will be deleted along with {@link #newNameResolver(URI, Attributes)} in + * a future release */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/5113") + @Deprecated public static final Attributes.Key PARAMS_PROXY_DETECTOR = Attributes.Key.create("params-proxy-detector"); @@ -214,6 +218,12 @@ public static abstract class CreationParams { */ public abstract int getDefaultPort(); + /** + * If the resolver wants to support proxy, it needs to use this proxy detector to generate + * {@link ProxySocketAddress}s. + */ + public abstract ProxyDetector getProxyDetector(); + public final String toString() { return MoreObjects.toStringHelper(this) .add("defaultPort", getDefaultPort()) From 33237fa3def23ced21d116c0220c680dcec2abea Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Sun, 10 Feb 2019 17:40:39 -0800 Subject: [PATCH 3/7] Rename CreationParams to Helper; All build passes; TODO: test backward compatibility --- core/src/main/java/io/grpc/NameResolver.java | 36 +++++++--------- .../java/io/grpc/NameResolverProvider.java | 4 +- .../AbstractManagedChannelImplBuilder.java | 11 ++--- .../io/grpc/internal/DnsNameResolver.java | 9 ++-- .../internal/DnsNameResolverProvider.java | 11 ++--- .../io/grpc/internal/ManagedChannelImpl.java | 39 +++++++++--------- .../OverrideAuthorityNameResolverFactory.java | 5 +-- .../io/grpc/NameResolverProviderTest.java | 21 +++++++--- .../internal/DnsNameResolverProviderTest.java | 21 ++++++---- .../io/grpc/internal/DnsNameResolverTest.java | 41 ++++++++++++------- ...ManagedChannelImplGetNameResolverTest.java | 25 +++++++---- .../ManagedChannelImplIdlenessTest.java | 4 +- .../grpc/internal/ManagedChannelImplTest.java | 21 +++++----- .../OverrideAuthorityNameResolverTest.java | 28 +++++++++---- .../io/grpc/netty/NettyChannelBuilder.java | 12 ++---- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 13 ++---- .../grpc/okhttp/OkHttpChannelBuilderTest.java | 4 +- 17 files changed, 163 insertions(+), 142 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 6225177a7a3..d7b8c3774d4 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -16,7 +16,6 @@ package io.grpc; -import com.google.common.base.MoreObjects; import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -124,20 +123,15 @@ public abstract static class Factory { * @param params optional parameters. Canonical keys are defined as {@code PARAMS_*} fields in * {@link Factory}. * - * @deprecated Implement {@link #newNameResolver(URI, Params)} instead. This is going to be - * deleted in a future release. + * @deprecated Implement {@link #newNameResolver(URI, NameResolver.Helper)} instead. This is + * going to be deleted in a future release. * * @since 1.0.0 */ @Nullable @Deprecated public NameResolver newNameResolver(URI targetUri, final Attributes params) { - return newNameResolver(targetUri, new CreationParams() { - @Override - public int getDefaultPort() { - return params.get(PARAMS_DEFAULT_PORT); - } - }); + throw new UnsupportedOperationException("This method is going to be deleted"); } /** @@ -146,15 +140,19 @@ public int getDefaultPort() { * URI. * * @param targetUri the target URI to be resolved, whose scheme must not be {@code null} - * @param params additional parameters that may be used by the NameResolver implementation + * @param helper utility that may be used by the NameResolver implementation * * @since 1.19.0 */ + // TODO(zhangkun83): make this abstract when the other override is deleted @Nullable - public NameResolver newNameResolver(URI targetUri, CreationParams params) { + public NameResolver newNameResolver(URI targetUri, Helper helper) { return newNameResolver( targetUri, - Attributes.newBuilder().set(PARAMS_DEFAULT_PORT, params.getDefaultPort()).build()); + Attributes.newBuilder() + .set(PARAMS_DEFAULT_PORT, helper.getDefaultPort()) + .set(PARAMS_PROXY_DETECTOR, helper.getProxyDetector()) + .build()); } /** @@ -209,9 +207,9 @@ void onAddresses( public @interface ResolutionResultAttr {} /** - * Parameters passed to {@link Factory#newNameResolver(URI, CreationParams)}. + * A utility object passed to {@link Factory#newNameResolver(URI, NameResolver.Helper)}. */ - public static abstract class CreationParams { + public abstract static class Helper { /** * The port number used in case the target or the underlying naming system doesn't provide a * port number. @@ -219,15 +217,9 @@ public static abstract class CreationParams { public abstract int getDefaultPort(); /** - * If the resolver wants to support proxy, it needs to use this proxy detector to generate - * {@link ProxySocketAddress}s. + * If the NameResolver wants to support proxy, it should inquire this {@link ProxyDetector}. + * See documentation on {@link ProxyDetector} about how proxies work in gRPC. */ public abstract ProxyDetector getProxyDetector(); - - public final String toString() { - return MoreObjects.toStringHelper(this) - .add("defaultPort", getDefaultPort()) - .toString(); - } } } diff --git a/core/src/main/java/io/grpc/NameResolverProvider.java b/core/src/main/java/io/grpc/NameResolverProvider.java index f4137e09540..0347a174aaa 100644 --- a/core/src/main/java/io/grpc/NameResolverProvider.java +++ b/core/src/main/java/io/grpc/NameResolverProvider.java @@ -107,10 +107,10 @@ private static final class NameResolverFactory extends NameResolver.Factory { @Override @Nullable - public NameResolver newNameResolver(URI targetUri, NameResolver.CreationParams params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { checkForProviders(); for (NameResolverProvider provider : providers) { - NameResolver resolver = provider.newNameResolver(targetUri, params); + NameResolver resolver = provider.newNameResolver(targetUri, helper); if (resolver != null) { return resolver; } diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 240d9fdc4cb..97c232e799c 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -488,11 +488,12 @@ final List getEffectiveInterceptors() { protected abstract ClientTransportFactory buildTransportFactory(); /** - * Subclasses can override this method to provide parameters to {@link - * NameResolver.Factory#newNameResolver}. The default implementation returns an + * 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 + * 443. */ - protected Attributes getNameResolverParams() { - return Attributes.EMPTY; + protected int getDefaultPort() { + return 443; } /** @@ -516,7 +517,7 @@ private static class DirectAddressNameResolverFactory extends NameResolver.Facto } @Override - public NameResolver newNameResolver(URI notUsedUri, NameResolver.CreationParams params) { + public NameResolver newNameResolver(URI notUsedUri, NameResolver.Helper helper) { return new NameResolver() { @Override public String getServiceAuthority() { diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 19325d5bdcb..4a740b816b5 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolver.java @@ -149,9 +149,8 @@ final class DnsNameResolver extends NameResolver { private final Runnable resolveRunnable; - DnsNameResolver(@Nullable String nsAuthority, String name, CreationParams params, - Resource executorResource, ProxyDetector proxyDetector, - Stopwatch stopwatch, boolean isAndroid) { + DnsNameResolver(@Nullable String nsAuthority, String name, Helper helper, + Resource executorResource, Stopwatch stopwatch, boolean isAndroid) { // TODO: if a DNS server is provided as nsAuthority, use it. // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java this.executorResource = executorResource; @@ -163,11 +162,11 @@ final class DnsNameResolver extends NameResolver { "nameUri (%s) doesn't have an authority", nameUri); host = nameUri.getHost(); if (nameUri.getPort() == -1) { - port = params.getDefaultPort(); + port = helper.getDefaultPort(); } else { port = nameUri.getPort(); } - this.proxyDetector = proxyDetector; + this.proxyDetector = Preconditions.checkNotNull(helper.getProxyDetector(), "proxyDetector"); this.resolveRunnable = new Resolve(this, stopwatch, getNetworkAddressCacheTtlNanos(isAndroid)); } diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index cc451252c8e..83eaef2e831 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -18,11 +18,9 @@ import com.google.common.base.Preconditions; import com.google.common.base.Stopwatch; -import io.grpc.Attributes; import io.grpc.InternalServiceProviders; -import io.grpc.NameResolver.Factory; +import io.grpc.NameResolver; import io.grpc.NameResolverProvider; -import io.grpc.ProxyDetector; import java.net.URI; /** @@ -45,20 +43,17 @@ public final class DnsNameResolverProvider extends NameResolverProvider { private static final String SCHEME = "dns"; @Override - public DnsNameResolver newNameResolver(URI targetUri, CreationParams params) { + public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { if (SCHEME.equals(targetUri.getScheme())) { String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath"); Preconditions.checkArgument(targetPath.startsWith("/"), "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri); String name = targetPath.substring(1); - ProxyDetector proxyDetector = Preconditions - .checkNotNull(params.get(Factory.PARAMS_PROXY_DETECTOR), "proxyDetector"); return new DnsNameResolver( targetUri.getAuthority(), name, - params, + helper, GrpcUtil.SHARED_CHANNEL_EXECUTOR, - proxyDetector, Stopwatch.createUnstarted(), InternalServiceProviders.isAndroid(getClass().getClassLoader())); } else { diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index b69c131481d..a6bb50463c1 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -126,7 +126,7 @@ final class ManagedChannelImpl extends ManagedChannel implements private final InternalLogId logId; private final String target; private final NameResolver.Factory nameResolverFactory; - private final NameResolver.CreationParams nameResolverParams; + private final NameResolver.Helper nameResolverHelper; private final LoadBalancer.Factory loadBalancerFactory; private final ClientTransportFactory transportFactory; private final ScheduledExecutorForBalancer scheduledExecutorForBalancer; @@ -136,7 +136,6 @@ final class ManagedChannelImpl extends ManagedChannel implements private final ExecutorHolder balancerRpcExecutorHolder; private final TimeProvider timeProvider; private final int maxTraceEvents; - private final ProxyDetector proxyDetector; @VisibleForTesting final SynchronizationContext syncContext = new SynchronizationContext( @@ -320,7 +319,7 @@ private void shutdownNameResolverAndLoadBalancer(boolean channelIsActive) { nameResolver.shutdown(); nameResolverStarted = false; if (channelIsActive) { - nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); + nameResolver = getNameResolver(target, nameResolverFactory, nameResolverHelper); } else { nameResolver = null; } @@ -545,11 +544,21 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new this.target = checkNotNull(builder.target, "target"); this.logId = InternalLogId.allocate("Channel", target); this.nameResolverFactory = builder.getNameResolverFactory(); - this.proxyDetector = + final ProxyDetector proxyDetector = builder.proxyDetector != null ? builder.proxyDetector : GrpcUtil.getDefaultProxyDetector(); - this.nameResolverParams = addProxyToAttributes(this.proxyDetector, - checkNotNull(builder.getNameResolverParams(), "nameResolverParams")); - this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); + final int defaultPort = builder.getDefaultPort(); + this.nameResolverHelper = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return defaultPort; + } + + @Override + public ProxyDetector getProxyDetector() { + return proxyDetector; + } + }; + this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverHelper); this.timeProvider = checkNotNull(timeProvider, "timeProvider"); maxTraceEvents = builder.maxTraceEvents; channelTracer = new ChannelTracer( @@ -617,19 +626,9 @@ public CallTracer create() { channelz.addRootChannel(this); } - private static Attributes addProxyToAttributes(ProxyDetector proxyDetector, - Attributes attributes) { - if (attributes.get(NameResolver.Factory.PARAMS_PROXY_DETECTOR) == null) { - return attributes.toBuilder() - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, proxyDetector).build(); - } else { - return attributes; - } - } - @VisibleForTesting static NameResolver getNameResolver(String target, NameResolver.Factory nameResolverFactory, - NameResolver.CreationParams nameResolverParams) { + NameResolver.Helper nameResolverHelper) { // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending // "dns:///". URI targetUri = null; @@ -644,7 +643,7 @@ static NameResolver getNameResolver(String target, NameResolver.Factory nameReso uriSyntaxErrors.append(e.getMessage()); } if (targetUri != null) { - NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams); + NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverHelper); if (resolver != null) { return resolver; } @@ -662,7 +661,7 @@ static NameResolver getNameResolver(String target, NameResolver.Factory nameReso // Should not be possible. throw new IllegalArgumentException(e); } - NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams); + NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverHelper); if (resolver != null) { return resolver; } diff --git a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java b/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java index 17494135592..ca75af764a5 100644 --- a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java +++ b/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java @@ -16,7 +16,6 @@ package io.grpc.internal; -import io.grpc.Attributes; import io.grpc.NameResolver; import java.net.URI; import javax.annotation.Nullable; @@ -43,8 +42,8 @@ final class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { @Nullable @Override - public NameResolver newNameResolver(URI targetUri, CreationParams params) { - final NameResolver resolver = delegate.newNameResolver(targetUri, params); + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { + final NameResolver resolver = delegate.newNameResolver(targetUri, helper); // Do not wrap null values. We do not want to impede error signaling. if (resolver == null) { return null; diff --git a/core/src/test/java/io/grpc/NameResolverProviderTest.java b/core/src/test/java/io/grpc/NameResolverProviderTest.java index 10a529b4c39..5b5f6a8a95c 100644 --- a/core/src/test/java/io/grpc/NameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/NameResolverProviderTest.java @@ -22,6 +22,8 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyZeroInteractions; import io.grpc.internal.DnsNameResolverProvider; import java.net.URI; @@ -29,6 +31,7 @@ import java.util.Iterator; import java.util.List; import java.util.concurrent.Callable; +import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -37,7 +40,13 @@ @RunWith(JUnit4.class) public class NameResolverProviderTest { private final URI uri = URI.create("dns:///localhost"); - private final Attributes attributes = Attributes.EMPTY; + private final NameResolver.Helper helper = mock(NameResolver.Helper.class); + + @After + public void wrapUp() { + // The helper is not implemented. Make sure it's not used in the test. + verifyZeroInteractions(helper); + } @Test public void getDefaultScheme_noProvider() { @@ -56,13 +65,13 @@ public void newNameResolver_providerReturnsNull() { List providers = Collections.singletonList( new BaseProvider(true, 5) { @Override - public NameResolver newNameResolver(URI passedUri, Attributes passedAttributes) { + public NameResolver newNameResolver(URI passedUri, NameResolver.Helper passedHelper) { assertSame(uri, passedUri); - assertSame(attributes, passedAttributes); + assertSame(helper, passedHelper); return null; } }); - assertNull(NameResolverProvider.asFactory(providers).newNameResolver(uri, attributes)); + assertNull(NameResolverProvider.asFactory(providers).newNameResolver(uri, helper)); } @Test @@ -70,7 +79,7 @@ public void newNameResolver_noProvider() { List providers = Collections.emptyList(); NameResolver.Factory factory = NameResolverProvider.asFactory(providers); try { - factory.newNameResolver(uri, attributes); + factory.newNameResolver(uri, helper); fail("Expected exception"); } catch (RuntimeException ex) { assertTrue(ex.toString(), ex.getMessage().contains("No NameResolverProviders found")); @@ -142,7 +151,7 @@ protected int priority() { } @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { throw new UnsupportedOperationException(); } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 273e6d2871c..4922d6350d8 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -20,8 +20,8 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; -import io.grpc.Attributes; import io.grpc.NameResolver; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,10 +31,17 @@ @RunWith(JUnit4.class) public class DnsNameResolverProviderTest { - private static final Attributes ATTRIBUTES = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + throw new UnsupportedOperationException("Should not be called"); + } + + @Override + public ProxyDetector getProxyDetector() { + return GrpcUtil.getDefaultProxyDetector(); + } + }; private DnsNameResolverProvider provider = new DnsNameResolverProvider(); @@ -46,8 +53,8 @@ public void isAvailable() { @Test public void newNameResolver() { assertSame(DnsNameResolver.class, - provider.newNameResolver(URI.create("dns:///localhost:443"), ATTRIBUTES).getClass()); + provider.newNameResolver(URI.create("dns:///localhost:443"), HELPER).getClass()); assertNull( - provider.newNameResolver(URI.create("notdns:///localhost:443"), ATTRIBUTES)); + provider.newNameResolver(URI.create("notdns:///localhost:443"), HELPER)); } } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index ae1d5206757..377ea92db85 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -95,11 +95,17 @@ public class DnsNameResolverTest { private final Map serviceConfig = new LinkedHashMap<>(); private static final int DEFAULT_PORT = 887; - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, DEFAULT_PORT) - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return DEFAULT_PORT; + } + + @Override + public ProxyDetector getProxyDetector() { + return GrpcUtil.getDefaultProxyDetector(); + } + }; private final DnsNameResolverProvider provider = new DnsNameResolverProvider(); private final FakeClock fakeClock = new FakeClock(); @@ -146,16 +152,25 @@ private DnsNameResolver newResolver( private DnsNameResolver newResolver( String name, - int port, - ProxyDetector proxyDetector, + final int port, + final ProxyDetector proxyDetector, Stopwatch stopwatch, boolean isAndroid) { DnsNameResolver dnsResolver = new DnsNameResolver( null, name, - Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, port).build(), + new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return port; + } + + @Override + public ProxyDetector getProxyDetector() { + return proxyDetector; + } + }, fakeExecutorResource, - proxyDetector, stopwatch, isAndroid); // By default, using the mocked ResourceResolver to avoid I/O @@ -279,9 +294,7 @@ private void resolveNeverCache(boolean isAndroid) throws Exception { public void resolveAll_failsOnEmptyResult() throws Exception { String hostname = "dns:///addr.fake:1234"; DnsNameResolver nrf = - new DnsNameResolverProvider().newNameResolver(new URI(hostname), Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build()); + new DnsNameResolverProvider().newNameResolver(new URI(hostname), HELPER); nrf.setAddressResolver(new AddressResolver() { @Override public List resolveAddress(String host) throws Exception { @@ -986,7 +999,7 @@ public void shouldUseJndi_trueIfItMightPossiblyBeValid() { private void testInvalidUri(URI uri) { try { - provider.newNameResolver(uri, NAME_RESOLVER_PARAMS); + provider.newNameResolver(uri, HELPER); fail("Should have failed"); } catch (IllegalArgumentException e) { // expected @@ -994,7 +1007,7 @@ private void testInvalidUri(URI uri) { } private void testValidUri(URI uri, String exportedAuthority, int expectedPort) { - DnsNameResolver resolver = provider.newNameResolver(uri, NAME_RESOLVER_PARAMS); + DnsNameResolver resolver = provider.newNameResolver(uri, HELPER); assertNotNull(resolver); assertEquals(expectedPort, resolver.getPort()); assertEquals(exportedAuthority, resolver.getServiceAuthority()); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java index 2fd3b9b694f..f08806102ad 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java @@ -20,9 +20,9 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; -import io.grpc.Attributes; import io.grpc.NameResolver; import io.grpc.NameResolver.Factory; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,8 +31,17 @@ /** Unit tests for {@link ManagedChannelImpl#getNameResolver}. */ @RunWith(JUnit4.class) public class ManagedChannelImplGetNameResolverTest { - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, 447).build(); + private static final NameResolver.Helper NAMERESOLVER_HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return 447; + } + + @Override + public ProxyDetector getProxyDetector() { + throw new UnsupportedOperationException("Should not be called"); + } + }; @Test public void invalidUriTarget() { @@ -96,7 +105,7 @@ public void validTargetStartingWithSlash() throws Exception { public void validTargetNoResovler() { Factory nameResolverFactory = new NameResolver.Factory() { @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { return null; } @@ -107,7 +116,7 @@ public String getDefaultScheme() { }; try { ManagedChannelImpl.getNameResolver( - "foo.googleapis.com:8080", nameResolverFactory, NAME_RESOLVER_PARAMS); + "foo.googleapis.com:8080", nameResolverFactory, NAMERESOLVER_HELPER); fail("Should fail"); } catch (IllegalArgumentException e) { // expected @@ -117,7 +126,7 @@ public String getDefaultScheme() { private void testValidTarget(String target, String expectedUriString, URI expectedUri) { Factory nameResolverFactory = new FakeNameResolverFactory(expectedUri.getScheme()); FakeNameResolver nameResolver = (FakeNameResolver) ManagedChannelImpl.getNameResolver( - target, nameResolverFactory, NAME_RESOLVER_PARAMS); + target, nameResolverFactory, NAMERESOLVER_HELPER); assertNotNull(nameResolver); assertEquals(expectedUri, nameResolver.uri); assertEquals(expectedUriString, nameResolver.uri.toString()); @@ -128,7 +137,7 @@ private void testInvalidTarget(String target) { try { FakeNameResolver nameResolver = (FakeNameResolver) ManagedChannelImpl.getNameResolver( - target, nameResolverFactory, NAME_RESOLVER_PARAMS); + target, nameResolverFactory, NAMERESOLVER_HELPER); fail("Should have failed, but got resolver with " + nameResolver.uri); } catch (IllegalArgumentException e) { // expected @@ -143,7 +152,7 @@ private static class FakeNameResolverFactory extends NameResolver.Factory { } @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { if (expectedScheme.equals(targetUri.getScheme())) { return new FakeNameResolver(targetUri); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 75cf2ee056b..e48a1c25277 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -142,7 +142,7 @@ public void setUp() { LoadBalancerRegistry.getDefaultRegistry().register(mockLoadBalancerProvider); when(mockNameResolver.getServiceAuthority()).thenReturn(AUTHORITY); when(mockNameResolverFactory - .newNameResolver(any(URI.class), any(Attributes.class))) + .newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(mockNameResolver); when(mockTransportFactory.getScheduledExecutorService()) .thenReturn(timer.getScheduledExecutorService()); @@ -181,7 +181,7 @@ builder, mockTransportFactory, new FakeBackoffPolicyProvider(), } servers.add(new EquivalentAddressGroup(addrs)); } - verify(mockNameResolverFactory).newNameResolver(any(URI.class), any(Attributes.class)); + verify(mockNameResolverFactory).newNameResolver(any(URI.class), any(NameResolver.Helper.class)); // Verify the initial idleness verify(mockLoadBalancerProvider, never()).newLoadBalancer(any(Helper.class)); verify(mockTransportFactory, never()).newClientTransport( diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 248a0692cea..f17ac07e77b 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -142,11 +142,7 @@ /** Unit tests for {@link ManagedChannelImpl}. */ @RunWith(JUnit4.class) public class ManagedChannelImplTest { - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, 447) - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final int DEFAULT_PORT = 447; private static final MethodDescriptor method = MethodDescriptor.newBuilder() @@ -3175,7 +3171,7 @@ final class FakeNameResolverFactory extends NameResolver.Factory { @Nullable @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { return (resolver = new FakeNameResolver()); } @@ -3240,6 +3236,11 @@ protected ClientTransportFactory buildTransportFactory() { mychannel.shutdownNow(); } + @Test + public void nameResolverParams_oldApi() { + // TODO(zhangkun83) + } + @Test public void getAuthorityAfterShutdown() throws Exception { createChannel(); @@ -3259,8 +3260,8 @@ private static final class ChannelBuilder throw new UnsupportedOperationException(); } - @Override protected Attributes getNameResolverParams() { - return NAME_RESOLVER_PARAMS; + @Override protected int getDefaultPort() { + return DEFAULT_PORT; } } @@ -3300,11 +3301,11 @@ private static final class FakeNameResolverFactory extends NameResolver.Factory } @Override - public NameResolver newNameResolver(final URI targetUri, Attributes params) { + public NameResolver newNameResolver(final URI targetUri, NameResolver.Helper helper) { if (!expectedUri.equals(targetUri)) { return null; } - assertSame(NAME_RESOLVER_PARAMS, params); + assertEquals(DEFAULT_PORT, helper.getDefaultPort()); FakeNameResolver resolver = new FakeNameResolver(error); resolvers.add(resolver); return resolver; diff --git a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java index 619b5ffc568..5c3ae49b154 100644 --- a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java @@ -23,8 +23,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import io.grpc.Attributes; import io.grpc.NameResolver; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,17 +33,28 @@ /** Unit tests for {@link OverrideAuthorityNameResolverFactory}. */ @RunWith(JUnit4.class) public class OverrideAuthorityNameResolverTest { + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + throw new UnsupportedOperationException("Should not be called"); + } + + @Override + public ProxyDetector getProxyDetector() { + throw new UnsupportedOperationException("Should not be called"); + } + }; + @Test public void overridesAuthority() { NameResolver nameResolverMock = mock(NameResolver.class); NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))) + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(nameResolverMock); String override = "override:5678"; NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, override); - NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), - Attributes.EMPTY); + NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER); assertNotNull(nameResolver); assertEquals(override, nameResolver.getServiceAuthority()); } @@ -51,23 +62,24 @@ public void overridesAuthority() { @Test public void wontWrapNull() { NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))).thenReturn(null); + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) + .thenReturn(null); NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, "override:5678"); assertEquals(null, - factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY)); + factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER)); } @Test public void forwardsNonOverridenCalls() { NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); NameResolver mockResolver = mock(NameResolver.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))) + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(mockResolver); NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, "override:5678"); NameResolver overrideResolver = - factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY); + factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER); assertNotNull(overrideResolver); NameResolver.Listener listener = mock(NameResolver.Listener.class); diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index af352dc2c3a..b2d68537c28 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -29,7 +29,6 @@ import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; import io.grpc.Internal; -import io.grpc.NameResolver; import io.grpc.ProxyParameters; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; @@ -415,21 +414,16 @@ eventLoopGroup, flowControlWindow, maxInboundMessageSize(), @Override @CheckReturnValue - protected Attributes getNameResolverParams() { - int defaultPort; + protected int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: case PLAINTEXT_UPGRADE: - defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT; - break; + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; case TLS: - defaultPort = GrpcUtil.DEFAULT_PORT_SSL; - break; + return GrpcUtil.DEFAULT_PORT_SSL; default: throw new AssertionError(negotiationType + " not handled"); } - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build(); } void overrideAuthorityChecker(@Nullable OverrideAuthorityChecker authorityChecker) { diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index acc75cda32d..036e6d41bac 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -23,10 +23,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import io.grpc.Attributes; import io.grpc.ExperimentalApi; import io.grpc.Internal; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; @@ -406,20 +404,15 @@ protected final ClientTransportFactory buildTransportFactory() { } @Override - protected Attributes getNameResolverParams() { - int defaultPort; + protected int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: - defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT; - break; + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; case TLS: - defaultPort = GrpcUtil.DEFAULT_PORT_SSL; - break; + return GrpcUtil.DEFAULT_PORT_SSL; default: throw new AssertionError(negotiationType + " not handled"); } - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build(); } @VisibleForTesting diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 993d5fe0b40..3f9104cf724 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -23,7 +23,6 @@ import static org.junit.Assert.assertSame; import com.squareup.okhttp.ConnectionSpec; -import io.grpc.NameResolver; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.FakeClock; import io.grpc.internal.GrpcUtil; @@ -120,8 +119,7 @@ public void usePlaintext_newClientTransportAllowed() { @Test public void usePlaintextDefaultPort() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("host", 1234).usePlaintext(); - assertEquals(GrpcUtil.DEFAULT_PORT_PLAINTEXT, - builder.getNameResolverParams().get(NameResolver.Factory.PARAMS_DEFAULT_PORT).intValue()); + assertEquals(GrpcUtil.DEFAULT_PORT_PLAINTEXT, builder.getDefaultPort()); } @Test From 92279e82944d6319cfea4deeb774b54f0958b1e8 Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Sun, 10 Feb 2019 18:13:01 -0800 Subject: [PATCH 4/7] Finish tests --- core/src/main/java/io/grpc/NameResolver.java | 2 +- .../grpc/internal/ManagedChannelImplTest.java | 86 ++++++++++++++++++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index d7b8c3774d4..94e5e8c8dcb 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -130,7 +130,7 @@ public abstract static class Factory { */ @Nullable @Deprecated - public NameResolver newNameResolver(URI targetUri, final Attributes params) { + public NameResolver newNameResolver(URI targetUri, Attributes params) { throw new UnsupportedOperationException("This method is going to be deleted"); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index f17ac07e77b..90db5c15d4e 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -89,6 +89,8 @@ import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.MethodType; import io.grpc.NameResolver; +import io.grpc.ProxyDetector; +import io.grpc.ProxyParameters; import io.grpc.SecurityLevel; import io.grpc.ServerMethodDefinition; import io.grpc.Status; @@ -3237,8 +3239,90 @@ protected ClientTransportFactory buildTransportFactory() { } @Test + public void nameResolverHelperPropagation() { + final AtomicReference capturedHelper = new AtomicReference<>(); + final NameResolver noopResolver = new NameResolver() { + @Override + public String getServiceAuthority() { + return "fake-authority"; + } + + @Override + public void start(Listener listener) { + } + + @Override + public void shutdown() {} + }; + ProxyDetector neverProxy = new ProxyDetector() { + @Override + public ProxyParameters proxyFor(SocketAddress targetAddress) { + return null; + } + }; + NameResolver.Factory oldApiFactory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { + capturedHelper.set(helper); + return noopResolver; + } + + @Override + public String getDefaultScheme() { + return "fakescheme"; + } + }; + channelBuilder.nameResolverFactory(oldApiFactory).proxyDetector(neverProxy); + createChannel(); + + NameResolver.Helper helper = capturedHelper.get(); + assertThat(helper).isNotNull(); + assertThat(helper.getDefaultPort()).isEqualTo(DEFAULT_PORT); + assertThat(helper.getProxyDetector()).isSameAs(neverProxy); + } + + @Test + @Deprecated public void nameResolverParams_oldApi() { - // TODO(zhangkun83) + final AtomicReference capturedParams = new AtomicReference<>(); + final NameResolver noopResolver = new NameResolver() { + @Override + public String getServiceAuthority() { + return "fake-authority"; + } + + @Override + public void start(Listener listener) { + } + + @Override + public void shutdown() {} + }; + ProxyDetector neverProxy = new ProxyDetector() { + @Override + public ProxyParameters proxyFor(SocketAddress targetAddress) { + return null; + } + }; + NameResolver.Factory oldApiFactory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, Attributes params) { + capturedParams.set(params); + return noopResolver; + } + + @Override + public String getDefaultScheme() { + return "fakescheme"; + } + }; + channelBuilder.nameResolverFactory(oldApiFactory).proxyDetector(neverProxy); + createChannel(); + + Attributes attrs = capturedParams.get(); + assertThat(attrs).isNotNull(); + assertThat(attrs.get(NameResolver.Factory.PARAMS_DEFAULT_PORT)).isEqualTo(DEFAULT_PORT); + assertThat(attrs.get(NameResolver.Factory.PARAMS_PROXY_DETECTOR)).isSameAs(neverProxy); } @Test From 3c4986493f6ebc1c181fcd36cc86d909cf404e26 Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Mon, 11 Feb 2019 11:11:30 -0800 Subject: [PATCH 5/7] Fix build --- .../test/java/io/grpc/internal/ManagedChannelImplTest.java | 6 +++--- netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 90db5c15d4e..7a4b6d87621 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -89,8 +89,8 @@ import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.MethodType; import io.grpc.NameResolver; +import io.grpc.ProxiedSocketAddress; import io.grpc.ProxyDetector; -import io.grpc.ProxyParameters; import io.grpc.SecurityLevel; import io.grpc.ServerMethodDefinition; import io.grpc.Status; @@ -3256,7 +3256,7 @@ public void shutdown() {} }; ProxyDetector neverProxy = new ProxyDetector() { @Override - public ProxyParameters proxyFor(SocketAddress targetAddress) { + public ProxiedSocketAddress proxyFor(SocketAddress targetAddress) { return null; } }; @@ -3300,7 +3300,7 @@ public void shutdown() {} }; ProxyDetector neverProxy = new ProxyDetector() { @Override - public ProxyParameters proxyFor(SocketAddress targetAddress) { + public ProxiedSocketAddress proxyFor(SocketAddress targetAddress) { return null; } }; diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index d6e831a8cce..bcb91a3046a 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -30,7 +30,6 @@ import io.grpc.ExperimentalApi; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; From c4fede3d2a92c71d9a702b764923436aef2e4b8d Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Mon, 11 Feb 2019 11:17:47 -0800 Subject: [PATCH 6/7] Refer to GrpcUtil.DEFAULT_PORT_SSL --- .../io/grpc/internal/AbstractManagedChannelImplBuilder.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 97c232e799c..b72a840b843 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -490,10 +490,10 @@ final List getEffectiveInterceptors() { /** * 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 - * 443. + * {@link GrpcUtil.DEFAULT_PORT_SSL}. */ protected int getDefaultPort() { - return 443; + return GrpcUtil.DEFAULT_PORT_SSL; } /** From c0e0a322b11bada183635a49c822a3e1d52998eb Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Tue, 12 Feb 2019 09:52:49 -0800 Subject: [PATCH 7/7] Fix cronet --- .../main/java/io/grpc/cronet/CronetChannelBuilder.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java index b91a9500367..c0d200ce019 100644 --- a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java +++ b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java @@ -23,9 +23,7 @@ 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.ExperimentalApi; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -193,12 +191,6 @@ protected final ClientTransportFactory buildTransportFactory() { transportTracerFactory.create()); } - @Override - protected Attributes getNameResolverParams() { - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, GrpcUtil.DEFAULT_PORT_SSL).build(); - } - @VisibleForTesting static class CronetTransportFactory implements ClientTransportFactory { private final ScheduledExecutorService timeoutService;