core: Remove RetryingNR.RESOLUTION_RESULT_LISTENER_KEY

It was introduced in fcb5c54 because at the time we didn't change the
API to communicate the status. When onResult2() was introduced in
90d0fab this hack stopped being necessary.

Author: ejona86

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -94,7 +94,6 @@
 import io.grpc.internal.ManagedChannelServiceConfig.ServiceConfigConvertedSelector;
 import io.grpc.internal.RetriableStream.ChannelBufferMeter;
 import io.grpc.internal.RetriableStream.Throttle;
-import io.grpc.internal.RetryingNameResolver.ResolutionResultListener;
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -1653,18 +1652,7 @@ final class NameResolverListener extends NameResolver.Listener2 {
 
     @Override
     public void onResult(final ResolutionResult resolutionResult) {
-      final class NamesResolved implements Runnable {
-
-        @Override
-        public void run() {
-          Status status = onResult2(resolutionResult);
-          ResolutionResultListener resolutionResultListener = resolutionResult.getAttributes()
-              .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
-          resolutionResultListener.resolutionAttempted(status);
-        }
-      }
-
-      syncContext.execute(new NamesResolved());
+      syncContext.execute(() -> onResult2(resolutionResult));
     }
 
     @SuppressWarnings("ReferenceEquality")

core/src/main/java/io/grpc/internal/RetryingNameResolver.java

@@ -17,7 +17,6 @@
 package io.grpc.internal;
 
 import com.google.common.annotations.VisibleForTesting;
-import io.grpc.Attributes;
 import io.grpc.NameResolver;
 import io.grpc.Status;
 import io.grpc.SynchronizationContext;
@@ -34,9 +33,6 @@ final class RetryingNameResolver extends ForwardingNameResolver {
   private final RetryScheduler retryScheduler;
   private final SynchronizationContext syncContext;
 
-  static final Attributes.Key<ResolutionResultListener> RESOLUTION_RESULT_LISTENER_KEY
-      = Attributes.Key.create(
-          "io.grpc.internal.RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY");
 
   /**
    * Creates a new {@link RetryingNameResolver}.
@@ -88,18 +84,7 @@ private class RetryingListener extends Listener2 {
 
     @Override
     public void onResult(ResolutionResult resolutionResult) {
-      // If the resolution result listener is already an attribute it indicates that a name resolver
-      // has already been wrapped with this class. This indicates a misconfiguration.
-      if (resolutionResult.getAttributes().get(RESOLUTION_RESULT_LISTENER_KEY) != null) {
-        throw new IllegalStateException(
-            "RetryingNameResolver can only be used once to wrap a NameResolver");
-      }
-
-      // To have retry behavior for name resolvers that haven't migrated to onResult2.
-      delegateListener.onResult(resolutionResult.toBuilder().setAttributes(
-              resolutionResult.getAttributes().toBuilder()
-                  .set(RESOLUTION_RESULT_LISTENER_KEY, new ResolutionResultListener()).build())
-          .build());
+      syncContext.execute(() -> onResult2(resolutionResult));
     }
 
     @Override
@@ -119,19 +104,4 @@ public void onError(Status error) {
       syncContext.execute(() -> retryScheduler.schedule(new DelayedNameResolverRefresh()));
     }
   }
-
-  /**
-   * Simple callback class to store in {@link ResolutionResult} attributes so that
-   * ManagedChannel can indicate if the resolved addresses were accepted. Temporary until
-   * the Listener2.onResult() API can be changed to return a boolean for this purpose.
-   */
-  class ResolutionResultListener {
-    public void resolutionAttempted(Status successStatus) {
-      if (successStatus.isOk()) {
-        retryScheduler.reset();
-      } else {
-        retryScheduler.schedule(new DelayedNameResolverRefresh());
-      }
-    }
-  }
 }

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

@@ -3844,8 +3844,6 @@ public double nextDouble() {
     verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
     ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
     assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
-    assertThat(resolvedAddresses.getAttributes()
-        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();
 
     // simulating request connection and then transport ready after resolved address
     Subchannel subchannel =
@@ -3951,8 +3949,6 @@ public void hedgingScheduledThenChannelShutdown_hedgeShouldStillHappen_newCallSh
     verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
     ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
     assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
-    assertThat(resolvedAddresses.getAttributes()
-        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();
 
     // simulating request connection and then transport ready after resolved address
     Subchannel subchannel =

core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java

@@ -17,7 +17,6 @@
 package io.grpc.internal;
 
 import static com.google.common.truth.Truth.assertThat;
-import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.isA;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -28,7 +27,6 @@
 import io.grpc.NameResolver.ResolutionResult;
 import io.grpc.Status;
 import io.grpc.SynchronizationContext;
-import io.grpc.internal.RetryingNameResolver.ResolutionResultListener;
 import java.lang.Thread.UncaughtExceptionHandler;
 import org.junit.Before;
 import org.junit.Rule;
@@ -58,8 +56,6 @@ public class RetryingNameResolverTest {
   private RetryScheduler mockRetryScheduler;
   @Captor
   private ArgumentCaptor<Listener2> listenerCaptor;
-  @Captor
-  private ArgumentCaptor<ResolutionResult> onResultCaptor;
   private final SynchronizationContext syncContext = new SynchronizationContext(
       mock(UncaughtExceptionHandler.class));
 
@@ -77,21 +73,14 @@ public void startAndShutdown() {
     retryingNameResolver.shutdown();
   }
 
-  // Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes,
-  // and the retry scheduler is reset since the name resolution was successful.
   @Test
   public void onResult_success() {
+    when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.OK);
     retryingNameResolver.start(mockListener);
     verify(mockNameResolver).start(listenerCaptor.capture());
 
     listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
-    verify(mockListener).onResult(onResultCaptor.capture());
-    ResolutionResultListener resolutionResultListener = onResultCaptor.getValue()
-        .getAttributes()
-        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
-    assertThat(resolutionResultListener).isNotNull();
 
-    resolutionResultListener.resolutionAttempted(Status.OK);
     verify(mockRetryScheduler).reset();
   }
 
@@ -107,21 +96,15 @@ public void onResult2_sucesss() {
     verify(mockRetryScheduler).reset();
   }
 
-  // Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes,
-  // and that a retry gets scheduled when the resolution results are rejected.
+  // Make sure that a retry gets scheduled when the resolution results are rejected.
   @Test
   public void onResult_failure() {
+    when(mockListener.onResult2(isA(ResolutionResult.class))).thenReturn(Status.UNAVAILABLE);
     retryingNameResolver.start(mockListener);
     verify(mockNameResolver).start(listenerCaptor.capture());
 
     listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
-    verify(mockListener).onResult(onResultCaptor.capture());
-    ResolutionResultListener resolutionResultListener = onResultCaptor.getValue()
-        .getAttributes()
-        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
-    assertThat(resolutionResultListener).isNotNull();
 
-    resolutionResultListener.resolutionAttempted(Status.UNAVAILABLE);
     verify(mockRetryScheduler).schedule(isA(Runnable.class));
   }
 
@@ -138,24 +121,6 @@ public void onResult2_failure() {
     verify(mockRetryScheduler).schedule(isA(Runnable.class));
   }
 
-  // Wrapping a NameResolver more than once is a misconfiguration.
-  @Test
-  public void onResult_failure_doubleWrapped() {
-    NameResolver doubleWrappedResolver = new RetryingNameResolver(retryingNameResolver,
-        mockRetryScheduler, syncContext);
-
-    doubleWrappedResolver.start(mockListener);
-    verify(mockNameResolver).start(listenerCaptor.capture());
-
-    try {
-      listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build());
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessageThat().contains("can only be used once");
-      return;
-    }
-    fail("An exception should have been thrown for a double wrapped NAmeResolver");
-  }
-
   // A retry should get scheduled when name resolution fails.
   @Test
   public void onError() {
@@ -165,4 +130,4 @@ public void onError() {
     verify(mockListener).onError(Status.DEADLINE_EXCEEDED);
     verify(mockRetryScheduler).schedule(isA(Runnable.class));
   }
-}
+}