core, grpclb: change policy selection strategy for Grpclb policy (take one: eliminate special logic for deciding grpclb policy in core)

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

@@ -223,6 +223,7 @@ public AsciiString scheme() {
       return SCHEME;
     }
 
+    @SuppressWarnings("deprecation")
     @Override
     public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
       ChannelHandler gnh = InternalProtocolNegotiators.grpcNegotiationHandler(grpcHandler);

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

@@ -41,18 +41,13 @@
 import io.grpc.Status;
 import io.grpc.internal.ServiceConfigUtil.LbConfig;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 // TODO(creamsoup) fully deprecate LoadBalancer.ATTR_LOAD_BALANCING_CONFIG
 @SuppressWarnings("deprecation")
 public final class AutoConfiguredLoadBalancerFactory {
-  private static final Logger logger =
-      Logger.getLogger(AutoConfiguredLoadBalancerFactory.class.getName());
-  private static final String GRPCLB_POLICY_NAME = "grpclb";
 
   private final LoadBalancerRegistry registry;
   private final String defaultPolicy;
@@ -92,7 +87,6 @@ public final class AutoConfiguredLoadBalancer {
     private final Helper helper;
     private LoadBalancer delegate;
     private LoadBalancerProvider delegateProvider;
-    private boolean roundRobinDueToGrpclbDepMissing;
 
     AutoConfiguredLoadBalancer(Helper helper) {
       this.helper = helper;
@@ -125,48 +119,53 @@ Status tryHandleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
       }
       PolicySelection policySelection =
           (PolicySelection) resolvedAddresses.getLoadBalancingPolicyConfig();
-      ResolvedPolicySelection resolvedSelection;
 
-      try {
-        resolvedSelection = resolveLoadBalancerProvider(servers, policySelection);
-      } catch (PolicyException e) {
-        Status s = Status.INTERNAL.withDescription(e.getMessage());
-        helper.updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, new FailingPicker(s));
-        delegate.shutdown();
-        delegateProvider = null;
-        delegate = new NoopLoadBalancer();
-        return Status.OK;
+      if (policySelection == null) {
+        LoadBalancerProvider defaultProvider;
+        try {
+          defaultProvider = getProviderOrThrow(defaultPolicy, "using default policy");
+        } catch (PolicyException e) {
+          Status s = Status.INTERNAL.withDescription(e.getMessage());
+          helper.updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, new FailingPicker(s));
+          delegate.shutdown();
+          delegateProvider = null;
+          delegate = new NoopLoadBalancer();
+          return Status.OK;
+        }
+        policySelection =
+            new PolicySelection(defaultProvider, /* rawConfig= */ null, /* config= */ null);
       }
-      PolicySelection selection = resolvedSelection.policySelection;
 
       if (delegateProvider == null
-          || !selection.provider.getPolicyName().equals(delegateProvider.getPolicyName())) {
+          || !policySelection.provider.getPolicyName().equals(delegateProvider.getPolicyName())) {
         helper.updateBalancingState(ConnectivityState.CONNECTING, new EmptyPicker());
         delegate.shutdown();
-        delegateProvider = selection.provider;
+        delegateProvider = policySelection.provider;
         LoadBalancer old = delegate;
         delegate = delegateProvider.newLoadBalancer(helper);
         helper.getChannelLogger().log(
             ChannelLogLevel.INFO, "Load balancer changed from {0} to {1}",
             old.getClass().getSimpleName(), delegate.getClass().getSimpleName());
       }
-      Object lbConfig = selection.config;
+      Object lbConfig = policySelection.config;
       if (lbConfig != null) {
         helper.getChannelLogger().log(
-            ChannelLogLevel.DEBUG, "Load-balancing config: {0}", selection.config);
+            ChannelLogLevel.DEBUG, "Load-balancing config: {0}", policySelection.config);
         attributes =
-            attributes.toBuilder().set(ATTR_LOAD_BALANCING_CONFIG, selection.rawConfig).build();
+            attributes.toBuilder()
+                .set(ATTR_LOAD_BALANCING_CONFIG, policySelection.rawConfig)
+                .build();
       }
 
       LoadBalancer delegate = getDelegate();
-      if (resolvedSelection.serverList.isEmpty()
+      if (resolvedAddresses.getAddresses().isEmpty()
           && !delegate.canHandleEmptyAddressListFromNameResolution()) {
         return Status.UNAVAILABLE.withDescription(
             "NameResolver returned no usable address. addrs=" + servers + ", attrs=" + attributes);
       } else {
         delegate.handleResolvedAddresses(
             ResolvedAddresses.newBuilder()
-                .setAddresses(resolvedSelection.serverList)
+                .setAddresses(resolvedAddresses.getAddresses())
                 .setAttributes(attributes)
                 .setLoadBalancingPolicyConfig(lbConfig)
                 .build());
@@ -206,78 +205,6 @@ void setDelegate(LoadBalancer lb) {
     LoadBalancerProvider getDelegateProvider() {
       return delegateProvider;
     }
-
-    /**
-     * Resolves a load balancer based on given criteria.  If policySelection is {@code null} and
-     * given servers contains any gRPC LB addresses, it will fall back to "grpclb". If no gRPC LB
-     * addresses are not present, it will fall back to {@link #defaultPolicy}.
-     *
-     * @param servers The list of servers reported
-     * @param policySelection the selected policy from raw service config
-     * @return the resolved policy selection
-     */
-    @VisibleForTesting
-    ResolvedPolicySelection resolveLoadBalancerProvider(
-        List<EquivalentAddressGroup> servers, @Nullable PolicySelection policySelection)
-        throws PolicyException {
-      // Check for balancer addresses
-      boolean haveBalancerAddress = false;
-      List<EquivalentAddressGroup> backendAddrs = new ArrayList<>();
-      for (EquivalentAddressGroup s : servers) {
-        if (s.getAttributes().get(GrpcAttributes.ATTR_LB_ADDR_AUTHORITY) != null) {
-          haveBalancerAddress = true;
-        } else {
-          backendAddrs.add(s);
-        }
-      }
-
-      if (policySelection != null) {
-        String policyName = policySelection.provider.getPolicyName();
-        return new ResolvedPolicySelection(
-            policySelection, policyName.equals(GRPCLB_POLICY_NAME) ? servers : backendAddrs);
-      }
-
-      if (haveBalancerAddress) {
-        // This is a special case where the existence of balancer address in the resolved address
-        // selects "grpclb" policy if the service config couldn't select a policy
-        LoadBalancerProvider grpclbProvider = registry.getProvider(GRPCLB_POLICY_NAME);
-        if (grpclbProvider == null) {
-          if (backendAddrs.isEmpty()) {
-            throw new PolicyException(
-                "Received ONLY balancer addresses but grpclb runtime is missing");
-          }
-          if (!roundRobinDueToGrpclbDepMissing) {
-            // We don't log the warning every time we have an update.
-            roundRobinDueToGrpclbDepMissing = true;
-            String errorMsg = "Found balancer addresses but grpclb runtime is missing."
-                + " Will use round_robin. Please include grpc-grpclb in your runtime dependencies.";
-            helper.getChannelLogger().log(ChannelLogLevel.ERROR, errorMsg);
-            logger.warning(errorMsg);
-          }
-          return new ResolvedPolicySelection(
-              new PolicySelection(
-                  getProviderOrThrow(
-                      "round_robin", "received balancer addresses but grpclb runtime is missing"),
-                  /* rawConfig = */ null,
-                  /* config= */ null),
-              backendAddrs);
-        }
-        return new ResolvedPolicySelection(
-            new PolicySelection(
-                grpclbProvider, /* rawConfig= */ null, /* config= */ null), servers);
-      }
-      // No balancer address this time.  If balancer address shows up later, we want to make sure
-      // the warning is logged one more time.
-      roundRobinDueToGrpclbDepMissing = false;
-
-      // No config nor balancer address. Use default.
-      return new ResolvedPolicySelection(
-          new PolicySelection(
-              getProviderOrThrow(defaultPolicy, "using default policy"),
-              /* rawConfig= */ null,
-              /* config= */ null),
-          servers);
-    }
   }
 
   private LoadBalancerProvider getProviderOrThrow(String policy, String choiceReason)
@@ -405,26 +332,6 @@ public String toString() {
     }
   }
 
-  @VisibleForTesting
-  static final class ResolvedPolicySelection {
-    final PolicySelection policySelection;
-    final List<EquivalentAddressGroup> serverList;
-
-    ResolvedPolicySelection(
-        PolicySelection policySelection, List<EquivalentAddressGroup> serverList) {
-      this.policySelection = checkNotNull(policySelection, "policySelection");
-      this.serverList = Collections.unmodifiableList(checkNotNull(serverList, "serverList"));
-    }
-
-    @Override
-    public String toString() {
-      return MoreObjects.toStringHelper(this)
-          .add("policySelection", policySelection)
-          .add("serverList", serverList)
-          .toString();
-    }
-  }
-
   private static final class EmptyPicker extends SubchannelPicker {
 
     @Override

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

@@ -298,14 +298,12 @@ public void run() {
       for (InetAddress inetAddr : resolutionResults.addresses) {
         servers.add(new EquivalentAddressGroup(new InetSocketAddress(inetAddr, port)));
       }
-      servers.addAll(resolutionResults.balancerAddresses);
-      if (servers.isEmpty()) {
-        savedListener.onError(Status.UNAVAILABLE.withDescription(
-            "No DNS backend or balancer addresses found for " + host));
-        return;
-      }
 
       ResolutionResult.Builder resultBuilder = ResolutionResult.newBuilder().setAddresses(servers);
+      Attributes.Builder attributesBuilder = Attributes.newBuilder();
+      if (!resolutionResults.balancerAddresses.isEmpty()) {
+        attributesBuilder.set(GrpcAttributes.ATTR_LB_ADDRS, resolutionResults.balancerAddresses);
+      }
       if (!resolutionResults.txtRecords.isEmpty()) {
         ConfigOrError rawServiceConfig =
             parseServiceConfig(resolutionResults.txtRecords, random, getLocalHostname());
@@ -319,17 +317,14 @@ public void run() {
           Map<String, ?> verifiedRawServiceConfig = (Map<String, ?>) rawServiceConfig.getConfig();
           ConfigOrError parsedServiceConfig =
               serviceConfigParser.parseServiceConfig(verifiedRawServiceConfig);
-          resultBuilder
-              .setAttributes(
-                  Attributes.newBuilder()
-                      .set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, verifiedRawServiceConfig)
-                      .build())
-              .setServiceConfig(parsedServiceConfig);
+          resultBuilder.setServiceConfig(parsedServiceConfig);
+          attributesBuilder
+              .set(GrpcAttributes.NAME_RESOLVER_SERVICE_CONFIG, verifiedRawServiceConfig);
         }
       } else {
         logger.log(Level.FINE, "No TXT records found for {0}", new Object[]{host});
       }
-      savedListener.onResult(resultBuilder.build());
+      savedListener.onResult(resultBuilder.setAttributes(attributesBuilder.build()).build());
     }
   }
 

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

@@ -21,6 +21,7 @@
 import io.grpc.Grpc;
 import io.grpc.NameResolver;
 import io.grpc.SecurityLevel;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -37,10 +38,23 @@ public final class GrpcAttributes {
   public static final Attributes.Key<Map<String, ?>> NAME_RESOLVER_SERVICE_CONFIG =
       Attributes.Key.create("service-config");
 
+  /**
+   * Attribute key for gRPC LB server addresses.
+   *
+   * <p>Deprecated: this will be used for grpclb specific logic, which will be moved out of core.
+   */
+  @Deprecated
+  @NameResolver.ResolutionResultAttr
+  public static final Attributes.Key<List<EquivalentAddressGroup>> ATTR_LB_ADDRS =
+      Attributes.Key.create("io.grpc.grpclb.lbAddrs");
+
   /**
    * The naming authority of a gRPC LB server address.  It is an address-group-level attribute,
    * present when the address group is a LoadBalancer.
+   *
+   * <p>Deprecated: this will be used for grpclb specific logic, which will be moved out of core.
    */
+  @Deprecated
   @EquivalentAddressGroup.Attr
   public static final Attributes.Key<String> ATTR_LB_ADDR_AUTHORITY =
       Attributes.Key.create("io.grpc.grpclb.lbAddrAuthority");

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

@@ -129,6 +129,7 @@ public List<String> resolveTxt(String serviceConfigHostname) throws NamingExcept
       return Collections.unmodifiableList(serviceConfigTxtRecords);
     }
 
+    @SuppressWarnings("deprecation")
     @Override
     public List<EquivalentAddressGroup> resolveSrv(
         AddressResolver addressResolver, String grpclbHostname) throws Exception {