From af2c16d30153ad58bf9f4f02ebc94fae8ea27aec Mon Sep 17 00:00:00 2001 From: Kun Zhang Date: Thu, 30 May 2019 09:16:38 -0700 Subject: [PATCH] api: deprecate Helper.updateSubchannelAddresses() and add equivalent on Subchannel (#5802) Resolves #5676 --- api/src/main/java/io/grpc/LoadBalancer.java | 17 +++++++++++++ .../test/java/io/grpc/LoadBalancerTest.java | 2 ++ .../io/grpc/internal/ManagedChannelImpl.java | 7 ++++++ .../grpc/internal/PickFirstLoadBalancer.java | 2 +- .../util/ForwardingLoadBalancerHelper.java | 1 + .../io/grpc/util/ForwardingSubchannel.java | 5 ++++ .../ManagedChannelImplIdlenessTest.java | 2 +- .../grpc/internal/ManagedChannelImplTest.java | 25 +++++++++++++++++++ .../internal/PickFirstLoadBalancerTest.java | 8 +++--- .../main/java/io/grpc/grpclb/GrpclbState.java | 2 +- .../grpc/grpclb/GrpclbLoadBalancerTest.java | 6 ++--- 11 files changed, 66 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/io/grpc/LoadBalancer.java b/api/src/main/java/io/grpc/LoadBalancer.java index 4e13d24099..1b390bf268 100644 --- a/api/src/main/java/io/grpc/LoadBalancer.java +++ b/api/src/main/java/io/grpc/LoadBalancer.java @@ -963,7 +963,9 @@ public abstract class LoadBalancer { * href="https://github.com/grpc/grpc-java/issues/5015">#5015 for the background. * * @since 1.4.0 + * @deprecated use {@link Subchannel#updateAddresses} instead */ + @Deprecated public final void updateSubchannelAddresses( Subchannel subchannel, EquivalentAddressGroup addrs) { checkNotNull(addrs, "addrs"); @@ -982,7 +984,9 @@ public abstract class LoadBalancer { * @throws IllegalArgumentException if {@code subchannel} was not returned from {@link * #createSubchannel} or {@code addrs} is empty * @since 1.14.0 + * @deprecated use {@link Subchannel#updateAddresses} instead */ + @Deprecated public void updateSubchannelAddresses( Subchannel subchannel, List addrs) { throw new UnsupportedOperationException(); @@ -1301,6 +1305,19 @@ public abstract class LoadBalancer { throw new UnsupportedOperationException(); } + /** + * Replaces the existing addresses used with this {@code Subchannel}. If the new and old + * addresses overlap, the Subchannel can continue using an existing connection. + * + *

It must be called from the Synchronization Context or will throw. + * + * @throws IllegalArgumentException if {@code addrs} is empty + * @since 1.22.0 + */ + public void updateAddresses(List addrs) { + throw new UnsupportedOperationException(); + } + /** * (Internal use only) returns an object that represents the underlying subchannel that is used * by the Channel for sending RPCs when this {@link Subchannel} is picked. This is an opaque diff --git a/api/src/test/java/io/grpc/LoadBalancerTest.java b/api/src/test/java/io/grpc/LoadBalancerTest.java index 5933fdd210..e1a7b9a5d7 100644 --- a/api/src/test/java/io/grpc/LoadBalancerTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerTest.java @@ -167,6 +167,7 @@ public class LoadBalancerTest { } } + @Deprecated @Test public void helper_updateSubchannelAddresses_delegates() { class OverrideUpdateSubchannel extends NoopHelper { @@ -187,6 +188,7 @@ public class LoadBalancerTest { assertThat(helper.ran).isTrue(); } + @Deprecated @Test(expected = UnsupportedOperationException.class) public void helper_updateSubchannelAddressesList_throws() { new NoopHelper().updateSubchannelAddresses(null, Arrays.asList(eag)); diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index d0cc13576f..66ada6de99 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -1127,6 +1127,7 @@ final class ManagedChannelImpl extends ManagedChannel implements syncContext.execute(new LoadBalancerRefreshNameResolution()); } + @Deprecated @Override public void updateSubchannelAddresses( LoadBalancer.Subchannel subchannel, List addrs) { @@ -1621,6 +1622,12 @@ final class ManagedChannelImpl extends ManagedChannel implements public ChannelLogger getChannelLogger() { return subchannelLogger; } + + @Override + public void updateAddresses(List addrs) { + syncContext.throwIfNotInThisSynchronizationContext(); + subchannel.updateAddresses(addrs); + } } @Override diff --git a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java index 2056543824..acf9b88324 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java @@ -64,7 +64,7 @@ final class PickFirstLoadBalancer extends LoadBalancer { helper.updateBalancingState(CONNECTING, new Picker(PickResult.withSubchannel(subchannel))); subchannel.requestConnection(); } else { - helper.updateSubchannelAddresses(subchannel, servers); + subchannel.updateAddresses(servers); } } diff --git a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java index c1c516d06c..95bf66c019 100644 --- a/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java +++ b/core/src/main/java/io/grpc/util/ForwardingLoadBalancerHelper.java @@ -51,6 +51,7 @@ public abstract class ForwardingLoadBalancerHelper extends LoadBalancer.Helper { return delegate().createSubchannel(args); } + @Deprecated @Override public void updateSubchannelAddresses( Subchannel subchannel, List addrs) { diff --git a/core/src/main/java/io/grpc/util/ForwardingSubchannel.java b/core/src/main/java/io/grpc/util/ForwardingSubchannel.java index de9a511232..51f2583186 100644 --- a/core/src/main/java/io/grpc/util/ForwardingSubchannel.java +++ b/core/src/main/java/io/grpc/util/ForwardingSubchannel.java @@ -74,6 +74,11 @@ public abstract class ForwardingSubchannel extends LoadBalancer.Subchannel { return delegate().getInternalSubchannel(); } + @Override + public void updateAddresses(List addrs) { + delegate().updateAddresses(addrs); + } + @Override public String toString() { return MoreObjects.toStringHelper(this).add("delegate", delegate()).toString(); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 8b21d25ccc..409d03c5c0 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -549,7 +549,7 @@ public class ManagedChannelImplIdlenessTest { new Runnable() { @Override public void run() { - helper.updateSubchannelAddresses(subchannel, addrs); + subchannel.updateAddresses(Collections.singletonList(addrs)); } }); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 4b4a201b25..6d4497f922 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -184,7 +184,16 @@ public class ManagedChannelImplTest { return "test-addr"; } }; + private final SocketAddress socketAddress2 = + new SocketAddress() { + @Override + public String toString() { + return "test-addr"; + } + }; private final EquivalentAddressGroup addressGroup = new EquivalentAddressGroup(socketAddress); + private final EquivalentAddressGroup addressGroup2 = + new EquivalentAddressGroup(Arrays.asList(socketAddress, socketAddress2)); private final FakeClock timer = new FakeClock(); private final FakeClock executor = new FakeClock(); private final FakeClock balancerRpcExecutor = new FakeClock(); @@ -1322,6 +1331,11 @@ public class ManagedChannelImplTest { .newClientTransport( eq(socketAddress), eq(clientTransportOptions), isA(TransportLogger.class)); + // updateAddresses() + updateAddressesSafely(helper, sub1, Collections.singletonList(addressGroup2)); + assertThat(((InternalSubchannel) sub1.getInternalSubchannel()).getAddressGroups()) + .isEqualTo(Collections.singletonList(addressGroup2)); + // shutdown() has a delay shutdownSafely(helper, sub1); timer.forwardTime(ManagedChannelImpl.SUBCHANNEL_SHUTDOWN_DELAY_SECONDS - 1, TimeUnit.SECONDS); @@ -4116,6 +4130,17 @@ public class ManagedChannelImplTest { }); } + private static void updateAddressesSafely( + Helper helper, final Subchannel subchannel, final List addrs) { + helper.getSynchronizationContext().execute( + new Runnable() { + @Override + public void run() { + subchannel.updateAddresses(addrs); + } + }); + } + private static void shutdownSafely( final Helper helper, final Subchannel subchannel) { helper.getSynchronizationContext().execute( diff --git a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java index 4b08b91169..5abe28cec2 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java @@ -172,6 +172,7 @@ public class PickFirstLoadBalancerTest { verify(mockSubchannel).requestConnection(); loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); + verify(mockSubchannel).updateAddresses(eq(servers)); verifyNoMoreInteractions(mockSubchannel); verify(mockHelper).createSubchannel(createArgsCaptor.capture()); @@ -179,8 +180,7 @@ public class PickFirstLoadBalancerTest { verify(mockHelper) .updateBalancingState(isA(ConnectivityState.class), isA(SubchannelPicker.class)); // Updating the subchannel addresses is unnecessary, but doesn't hurt anything - verify(mockHelper).updateSubchannelAddresses( - eq(mockSubchannel), ArgumentMatchers.anyList()); + verify(mockSubchannel).updateAddresses(ArgumentMatchers.anyList()); verifyNoMoreInteractions(mockHelper); } @@ -191,7 +191,7 @@ public class PickFirstLoadBalancerTest { List newServers = Lists.newArrayList(new EquivalentAddressGroup(socketAddr)); - InOrder inOrder = inOrder(mockHelper); + InOrder inOrder = inOrder(mockHelper, mockSubchannel); loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build()); @@ -205,7 +205,7 @@ public class PickFirstLoadBalancerTest { loadBalancer.handleResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build()); - inOrder.verify(mockHelper).updateSubchannelAddresses(eq(mockSubchannel), eq(newServers)); + inOrder.verify(mockSubchannel).updateAddresses(eq(newServers)); verifyNoMoreInteractions(mockSubchannel); verifyNoMoreInteractions(mockHelper); diff --git a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java index a383d4643c..260b90912f 100644 --- a/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java +++ b/grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java @@ -438,7 +438,7 @@ final class GrpclbState { } else { checkState(subchannels.size() == 1, "Unexpected Subchannel count: %s", subchannels); subchannel = subchannels.values().iterator().next(); - helper.updateSubchannelAddresses(subchannel, eagList); + subchannel.updateAddresses(eagList); } subchannels = Collections.singletonMap(eagList, subchannel); newBackendList.add( diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java index 8d64e4df8b..db0c37c877 100644 --- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java +++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java @@ -1776,8 +1776,7 @@ public class GrpclbLoadBalancerTest { // createSubchannel() has ever been called only once verify(helper, times(1)).createSubchannel(any(List.class), any(Attributes.class)); assertThat(mockSubchannels).isEmpty(); - inOrder.verify(helper).updateSubchannelAddresses( - same(subchannel), + verify(subchannel).updateAddresses( eq(Arrays.asList( new EquivalentAddressGroup(backends2.get(0).addr, eagAttrsWithToken("token0001")), new EquivalentAddressGroup(backends2.get(2).addr, @@ -1874,8 +1873,7 @@ public class GrpclbLoadBalancerTest { // createSubchannel() has ever been called only once verify(helper, times(1)).createSubchannel(any(List.class), any(Attributes.class)); assertThat(mockSubchannels).isEmpty(); - inOrder.verify(helper).updateSubchannelAddresses( - same(subchannel), + verify(subchannel).updateAddresses( eq(Arrays.asList( new EquivalentAddressGroup(backends1.get(0).addr, eagAttrsWithToken("token0001")), new EquivalentAddressGroup(backends1.get(1).addr,