Merge branch 'grpc:master' into master

Author: kannanjgithub

api/src/main/java/io/grpc/NameResolver.java

@@ -303,6 +303,7 @@ public static final class Args {
     @Nullable private final Executor executor;
     @Nullable private final String overrideAuthority;
     @Nullable private final MetricRecorder metricRecorder;
+    @Nullable private final NameResolverRegistry nameResolverRegistry;
     @Nullable private final IdentityHashMap<Key<?>, Object> customArgs;
 
     private Args(Builder builder) {
@@ -316,6 +317,7 @@ private Args(Builder builder) {
       this.executor = builder.executor;
       this.overrideAuthority = builder.overrideAuthority;
       this.metricRecorder = builder.metricRecorder;
+      this.nameResolverRegistry = builder.nameResolverRegistry;
       this.customArgs = cloneCustomArgs(builder.customArgs);
     }
 
@@ -447,6 +449,18 @@ public MetricRecorder getMetricRecorder() {
       return metricRecorder;
     }
 
+    /**
+     * Returns the {@link NameResolverRegistry} that the Channel uses to look for {@link
+     * NameResolver}s.
+     *
+     * @since 1.74.0
+     */
+    public NameResolverRegistry getNameResolverRegistry() {
+      if (nameResolverRegistry == null) {
+        throw new IllegalStateException("NameResolverRegistry is not set in Builder");
+      }
+      return nameResolverRegistry;
+    }
 
     @Override
     public String toString() {
@@ -461,6 +475,7 @@ public String toString() {
           .add("executor", executor)
           .add("overrideAuthority", overrideAuthority)
           .add("metricRecorder", metricRecorder)
+          .add("nameResolverRegistry", nameResolverRegistry)
           .toString();
     }
 
@@ -480,6 +495,7 @@ public Builder toBuilder() {
       builder.setOffloadExecutor(executor);
       builder.setOverrideAuthority(overrideAuthority);
       builder.setMetricRecorder(metricRecorder);
+      builder.setNameResolverRegistry(nameResolverRegistry);
       builder.customArgs = cloneCustomArgs(customArgs);
       return builder;
     }
@@ -508,6 +524,7 @@ public static final class Builder {
       private Executor executor;
       private String overrideAuthority;
       private MetricRecorder metricRecorder;
+      private NameResolverRegistry nameResolverRegistry;
       private IdentityHashMap<Key<?>, Object> customArgs;
 
       Builder() {
@@ -614,6 +631,16 @@ public Builder setMetricRecorder(MetricRecorder metricRecorder) {
         return this;
       }
 
+      /**
+       * See {@link Args#getNameResolverRegistry}.  This is an optional field.
+       *
+       * @since 1.74.0
+       */
+      public Builder setNameResolverRegistry(NameResolverRegistry registry) {
+        this.nameResolverRegistry = registry;
+        return this;
+      }
+
       /**
        * Builds an {@link Args}.
        *

binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java

@@ -27,6 +27,7 @@
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.concurrent.GuardedBy;
 import com.google.protobuf.Empty;
 import io.grpc.CallOptions;
@@ -154,17 +155,20 @@ private class BinderClientTransportBuilder {
             .setScheduledExecutorPool(executorServicePool)
             .setOffloadExecutorPool(offloadServicePool);
 
+    @CanIgnoreReturnValue
     public BinderClientTransportBuilder setSecurityPolicy(SecurityPolicy securityPolicy) {
       factoryBuilder.setSecurityPolicy(securityPolicy);
       return this;
     }
 
+    @CanIgnoreReturnValue
     public BinderClientTransportBuilder setBinderDecorator(
         OneWayBinderProxy.Decorator binderDecorator) {
       factoryBuilder.setBinderDecorator(binderDecorator);
       return this;
     }
 
+    @CanIgnoreReturnValue
     public BinderClientTransportBuilder setReadyTimeoutMillis(int timeoutMillis) {
       factoryBuilder.setReadyTimeoutMillis(timeoutMillis);
       return this;

binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java

@@ -357,7 +357,7 @@ public void testIsDeviceOwner_failsWhenNoPackagesForUid() throws Exception {
   }
 
   @Test
-  @Config(sdk = 21)
+  @Config(sdk = Config.OLDEST_SDK)
   public void testIsProfileOwner_succeedsForProfileOwner() throws Exception {
     PackageInfo info =
         newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build();
@@ -371,7 +371,7 @@ public void testIsProfileOwner_succeedsForProfileOwner() throws Exception {
   }
 
   @Test
-  @Config(sdk = 21)
+  @Config(sdk = Config.OLDEST_SDK)
   public void testIsProfileOwner_failsForNotProfileOwner() throws Exception {
     PackageInfo info =
         newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build();
@@ -385,7 +385,7 @@ public void testIsProfileOwner_failsForNotProfileOwner() throws Exception {
   }
 
   @Test
-  @Config(sdk = 21)
+  @Config(sdk = Config.OLDEST_SDK)
   public void testIsProfileOwner_failsWhenNoPackagesForUid() throws Exception {
     policy = SecurityPolicies.isProfileOwner(appContext);
 
@@ -425,7 +425,7 @@ public void testIsProfileOwnerOnOrgOwned_failsForProfileOwnerOnNonOrgOwned() thr
   }
 
   @Test
-  @Config(sdk = 21)
+  @Config(sdk = Config.OLDEST_SDK)
   public void testIsProfileOwnerOnOrgOwned_failsForNotProfileOwner() throws Exception {
     PackageInfo info =
         newBuilder().setPackageName(OTHER_UID_PACKAGE_NAME).setSignatures(SIG2).build();
@@ -439,7 +439,7 @@ public void testIsProfileOwnerOnOrgOwned_failsForNotProfileOwner() throws Except
   }
 
   @Test
-  @Config(sdk = 21)
+  @Config(sdk = Config.OLDEST_SDK)
   public void testIsProfileOwnerOnOrgOwned_failsWhenNoPackagesForUid() throws Exception {
     policy = SecurityPolicies.isProfileOwnerOnOrganizationOwnedDevice(appContext);
 

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;
@@ -598,7 +597,8 @@ ClientStream newSubstream(
             .setChannelLogger(channelLogger)
             .setOffloadExecutor(this.offloadExecutorHolder)
             .setOverrideAuthority(this.authorityOverride)
-            .setMetricRecorder(this.metricRecorder);
+            .setMetricRecorder(this.metricRecorder)
+            .setNameResolverRegistry(builder.nameResolverRegistry);
     builder.copyAllNameResolverCustomArgsTo(nameResolverArgsBuilder);
     this.nameResolverArgs = nameResolverArgsBuilder.build();
     this.nameResolver = getNameResolver(
@@ -686,11 +686,7 @@ static NameResolver getNameResolver(
     // We wrap the name resolver in a RetryingNameResolver to give it the ability to retry failures.
     // TODO: After a transition period, all NameResolver implementations that need retry should use
     //       RetryingNameResolver directly and this step can be removed.
-    NameResolver usedNameResolver = new RetryingNameResolver(resolver,
-          new BackoffPolicyRetryScheduler(new ExponentialBackoffPolicy.Provider(),
-              nameResolverArgs.getScheduledExecutorService(),
-              nameResolverArgs.getSynchronizationContext()),
-          nameResolverArgs.getSynchronizationContext());
+    NameResolver usedNameResolver = RetryingNameResolver.wrap(resolver, nameResolverArgs);
 
     if (overrideAuthority == null) {
       return usedNameResolver;
@@ -1653,18 +1649,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;
@@ -28,16 +27,22 @@
  *
  * <p>The {@link NameResolver} used with this
  */
-final class RetryingNameResolver extends ForwardingNameResolver {
+public final class RetryingNameResolver extends ForwardingNameResolver {
+  public static NameResolver wrap(NameResolver retriedNameResolver, Args args) {
+    // For migration, this might become conditional
+    return new RetryingNameResolver(
+        retriedNameResolver,
+        new BackoffPolicyRetryScheduler(
+            new ExponentialBackoffPolicy.Provider(),
+            args.getScheduledExecutorService(),
+            args.getSynchronizationContext()),
+        args.getSynchronizationContext());
+  }
 
   private final NameResolver retriedNameResolver;
   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 +93,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 +113,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/DnsNameResolverTest.java

@@ -207,14 +207,7 @@ private RetryingNameResolver newResolver(
 
     // In practice the DNS name resolver provider always wraps the resolver in a
     // RetryingNameResolver which adds retry capabilities to it. We use the same setup here.
-    return new RetryingNameResolver(
-        dnsResolver,
-        new BackoffPolicyRetryScheduler(
-            new ExponentialBackoffPolicy.Provider(),
-            fakeExecutor.getScheduledExecutorService(),
-            syncContext
-        ),
-        syncContext);
+    return (RetryingNameResolver) RetryingNameResolver.wrap(dnsResolver, args);
   }
 
   @Before

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 =