From 20506a9430263d2002771be0fc95ef3e6f689f1a Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Mon, 8 Apr 2019 12:02:54 -0700 Subject: [PATCH 1/2] core: make newNameResolver() change backward compatible for callers. (#5560) We assumed that we were the only caller. Turns out there are forwarding NameResolver too. Implementing the old override will give them time to migrate to the new API. Resolves #5556 --- core/src/main/java/io/grpc/NameResolver.java | 25 +++++++- .../grpc/internal/ManagedChannelImplTest.java | 59 +++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 3d8bd5243aa..71a54c4af8f 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -115,6 +115,10 @@ public abstract static class Factory { public static final Attributes.Key PARAMS_PROXY_DETECTOR = Attributes.Key.create("params-proxy-detector"); + @Deprecated + private static final Attributes.Key PARAMS_SYNC_CONTEXT = + Attributes.Key.create("params-sync-context"); + /** * 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 @@ -131,8 +135,24 @@ public abstract static class Factory { */ @Nullable @Deprecated - public NameResolver newNameResolver(URI targetUri, Attributes params) { - throw new UnsupportedOperationException("This method is going to be deleted"); + public NameResolver newNameResolver(URI targetUri, final Attributes params) { + Helper helper = new Helper() { + @Override + public int getDefaultPort() { + return checkNotNull(params.get(PARAMS_DEFAULT_PORT), "default port not available"); + } + + @Override + public ProxyDetector getProxyDetector() { + return checkNotNull(params.get(PARAMS_PROXY_DETECTOR), "proxy detector not available"); + } + + @Override + public SynchronizationContext getSynchronizationContext() { + return checkNotNull(params.get(PARAMS_SYNC_CONTEXT), "sync context not available"); + } + }; + return newNameResolver(targetUri, helper); } /** @@ -153,6 +173,7 @@ public NameResolver newNameResolver(URI targetUri, Helper helper) { Attributes.newBuilder() .set(PARAMS_DEFAULT_PORT, helper.getDefaultPort()) .set(PARAMS_PROXY_DETECTOR, helper.getProxyDetector()) + .set(PARAMS_SYNC_CONTEXT, helper.getSynchronizationContext()) .build()); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 7a4b6d87621..b21b320cfc2 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -3325,6 +3325,65 @@ public String getDefaultScheme() { assertThat(attrs.get(NameResolver.Factory.PARAMS_PROXY_DETECTOR)).isSameAs(neverProxy); } + @Test + @Deprecated + public void nameResolverParams_forwardingResolverWithOldApi() { + 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 ProxiedSocketAddress proxyFor(SocketAddress targetAddress) { + return null; + } + }; + final NameResolver.Factory factory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { + capturedHelper.set(helper); + return noopResolver; + } + + @Override + public String getDefaultScheme() { + return "fakescheme"; + } + }; + + // A forwarding factory still with the old API can forward to a delegate factory that has + // migrated to the new API. + NameResolver.Factory oldApiForwardingFactory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, Attributes params) { + return factory.newNameResolver(targetUri, params); + } + + @Override + public String getDefaultScheme() { + return factory.getDefaultScheme(); + } + }; + channelBuilder.nameResolverFactory(oldApiForwardingFactory).proxyDetector(neverProxy); + createChannel(); + + NameResolver.Helper helper = capturedHelper.get(); + assertThat(helper).isNotNull(); + assertThat(helper.getDefaultPort()).isEqualTo(DEFAULT_PORT); + assertThat(helper.getProxyDetector()).isSameAs(neverProxy); + assertThat(helper.getSynchronizationContext()).isSameAs(channel.syncContext); + } + @Test public void getAuthorityAfterShutdown() throws Exception { createChannel(); From afbf6d533650516c27103051e0574a38f42f3a0c Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Tue, 9 Apr 2019 17:43:45 -0700 Subject: [PATCH 2/2] Fix build --- core/src/main/java/io/grpc/NameResolver.java | 12 ++---------- .../io/grpc/internal/ManagedChannelImplTest.java | 1 - 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index 71a54c4af8f..70db8efe613 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -16,6 +16,8 @@ package io.grpc; +import static com.google.common.base.Preconditions.checkNotNull; + import java.lang.annotation.Documented; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -115,10 +117,6 @@ public abstract static class Factory { public static final Attributes.Key PARAMS_PROXY_DETECTOR = Attributes.Key.create("params-proxy-detector"); - @Deprecated - private static final Attributes.Key PARAMS_SYNC_CONTEXT = - Attributes.Key.create("params-sync-context"); - /** * 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 @@ -146,11 +144,6 @@ public int getDefaultPort() { public ProxyDetector getProxyDetector() { return checkNotNull(params.get(PARAMS_PROXY_DETECTOR), "proxy detector not available"); } - - @Override - public SynchronizationContext getSynchronizationContext() { - return checkNotNull(params.get(PARAMS_SYNC_CONTEXT), "sync context not available"); - } }; return newNameResolver(targetUri, helper); } @@ -173,7 +166,6 @@ public NameResolver newNameResolver(URI targetUri, Helper helper) { Attributes.newBuilder() .set(PARAMS_DEFAULT_PORT, helper.getDefaultPort()) .set(PARAMS_PROXY_DETECTOR, helper.getProxyDetector()) - .set(PARAMS_SYNC_CONTEXT, helper.getSynchronizationContext()) .build()); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index b21b320cfc2..9b60898a3b5 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -3381,7 +3381,6 @@ public String getDefaultScheme() { assertThat(helper).isNotNull(); assertThat(helper.getDefaultPort()).isEqualTo(DEFAULT_PORT); assertThat(helper.getProxyDetector()).isSameAs(neverProxy); - assertThat(helper.getSynchronizationContext()).isSameAs(channel.syncContext); } @Test