all: refactor select lb policy from a list of raw configs

Refactor to reuse `PolicySelection` and the implementation in `AutoConfiguredLoadBalancerFactory.parseLoadBalancerPolicy()`.

Author: dapengzhang0

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

@@ -21,7 +21,6 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
-import com.google.common.base.Objects;
 import io.grpc.Attributes;
 import io.grpc.ChannelLogger;
 import io.grpc.ChannelLogger.ChannelLogLevel;
@@ -40,7 +39,7 @@
 import io.grpc.NameResolver.ConfigOrError;
 import io.grpc.Status;
 import io.grpc.internal.ServiceConfigUtil.LbConfig;
-import java.util.ArrayList;
+import io.grpc.internal.ServiceConfigUtil.PolicySelection;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -247,31 +246,7 @@ ConfigOrError parseLoadBalancerPolicy(Map<String, ?> serviceConfig, ChannelLogge
         loadBalancerConfigs = ServiceConfigUtil.unwrapLoadBalancingConfigList(rawLbConfigs);
       }
       if (loadBalancerConfigs != null && !loadBalancerConfigs.isEmpty()) {
-        List<String> policiesTried = new ArrayList<>();
-        for (LbConfig lbConfig : loadBalancerConfigs) {
-          String policy = lbConfig.getPolicyName();
-          LoadBalancerProvider provider = registry.getProvider(policy);
-          if (provider == null) {
-            policiesTried.add(policy);
-          } else {
-            if (!policiesTried.isEmpty()) {
-              channelLogger.log(
-                  ChannelLogLevel.DEBUG,
-                  "{0} specified by Service Config are not available", policiesTried);
-            }
-            ConfigOrError parsedLbPolicyConfig =
-                provider.parseLoadBalancingPolicyConfig(lbConfig.getRawConfigValue());
-            if (parsedLbPolicyConfig.getError() != null) {
-              return parsedLbPolicyConfig;
-            }
-            return ConfigOrError.fromConfig(
-                new PolicySelection(
-                    provider, lbConfig.getRawConfigValue(), parsedLbPolicyConfig.getConfig()));
-          }
-        }
-        return ConfigOrError.fromError(
-            Status.UNKNOWN.withDescription(
-                "None of " + policiesTried + " specified by Service Config are available."));
+        return ServiceConfigUtil.selectLbPolicyFromList(loadBalancerConfigs, registry);
       }
       return null;
     } catch (RuntimeException e) {
@@ -289,50 +264,6 @@ private PolicyException(String msg) {
     }
   }
 
-  @VisibleForTesting
-  static final class PolicySelection {
-    final LoadBalancerProvider provider;
-    @Nullable final Map<String, ?> rawConfig;
-    @Nullable final Object config;
-
-    PolicySelection(
-        LoadBalancerProvider provider,
-        @Nullable Map<String, ?> rawConfig,
-        @Nullable Object config) {
-      this.provider = checkNotNull(provider, "provider");
-      this.rawConfig = rawConfig;
-      this.config = config;
-    }
-
-    @Override
-    public boolean equals(Object o) {
-      if (this == o) {
-        return true;
-      }
-      if (o == null || getClass() != o.getClass()) {
-        return false;
-      }
-      PolicySelection that = (PolicySelection) o;
-      return Objects.equal(provider, that.provider)
-          && Objects.equal(rawConfig, that.rawConfig)
-          && Objects.equal(config, that.config);
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hashCode(provider, rawConfig, config);
-    }
-
-    @Override
-    public String toString() {
-      return MoreObjects.toStringHelper(this)
-          .add("provider", provider)
-          .add("rawConfig", rawConfig)
-          .add("config", config)
-          .toString();
-    }
-  }
-
   private static final class EmptyPicker extends SubchannelPicker {
 
     @Override

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

@@ -24,6 +24,9 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
 import com.google.common.base.VerifyException;
+import io.grpc.LoadBalancerProvider;
+import io.grpc.LoadBalancerRegistry;
+import io.grpc.NameResolver.ConfigOrError;
 import io.grpc.Status;
 import io.grpc.internal.RetriableStream.Throttle;
 import java.util.ArrayList;
@@ -33,6 +36,8 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 /**
@@ -323,6 +328,39 @@ public static List<LbConfig> unwrapLoadBalancingConfigList(List<Map<String, ?>>
     return Collections.unmodifiableList(result);
   }
 
+  /**
+   * Parses and selects a load balancing policy from a non-empty list of raw configs. If selection
+   * is successful, the returned ConfigOrError object will include a {@link
+   * ServiceConfigUtil.PolicySelection} as its config value.
+   */
+  public static ConfigOrError selectLbPolicyFromList(
+      List<LbConfig> lbConfigs, LoadBalancerRegistry lbRegistry) {
+    List<String> policiesTried = new ArrayList<>();
+    for (LbConfig lbConfig : lbConfigs) {
+      String policy = lbConfig.getPolicyName();
+      LoadBalancerProvider provider = lbRegistry.getProvider(policy);
+      if (provider == null) {
+        policiesTried.add(policy);
+      } else {
+        if (!policiesTried.isEmpty()) {
+          Logger.getLogger(ServiceConfigUtil.class.getName()).log(
+              Level.FINEST,
+              "{0} specified by Service Config are not available", policiesTried);
+        }
+        ConfigOrError parsedLbPolicyConfig =
+            provider.parseLoadBalancingPolicyConfig(lbConfig.getRawConfigValue());
+        if (parsedLbPolicyConfig.getError() != null) {
+          return parsedLbPolicyConfig;
+        }
+        return ConfigOrError.fromConfig(new PolicySelection(
+            provider, lbConfig.rawConfigValue, parsedLbPolicyConfig.getConfig()));
+      }
+    }
+    return ConfigOrError.fromError(
+        Status.UNKNOWN.withDescription(
+            "None of " + policiesTried + " specified by Service Config are available."));
+  }
+
   /**
    * A LoadBalancingConfig that includes the policy name (the key) and its raw config value (parsed
    * JSON).
@@ -367,4 +405,61 @@ public String toString() {
           .toString();
     }
   }
+
+  public static final class PolicySelection {
+    final LoadBalancerProvider provider;
+    @Deprecated
+    @Nullable
+    final Map<String, ?> rawConfig;
+    @Nullable
+    final Object config;
+
+    /** Constructs a PolicySelection with selected LB provider, a copy of raw config and the deeply
+     * parsed LB config. */
+    public PolicySelection(
+        LoadBalancerProvider provider,
+        @Nullable Map<String, ?> rawConfig,
+        @Nullable Object config) {
+      this.provider = checkNotNull(provider, "provider");
+      this.rawConfig = rawConfig;
+      this.config = config;
+    }
+
+    public LoadBalancerProvider getProvider() {
+      return provider;
+    }
+
+    @Nullable
+    public Object getConfig() {
+      return config;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
+      PolicySelection that = (PolicySelection) o;
+      return Objects.equal(provider, that.provider)
+          && Objects.equal(rawConfig, that.rawConfig)
+          && Objects.equal(config, that.config);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(provider, rawConfig, config);
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this)
+          .add("provider", provider)
+          .add("rawConfig", rawConfig)
+          .add("config", config)
+          .toString();
+    }
+  }
 }

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

@@ -51,11 +51,10 @@
 import io.grpc.Status;
 import io.grpc.grpclb.GrpclbLoadBalancerProvider;
 import io.grpc.internal.AutoConfiguredLoadBalancerFactory.AutoConfiguredLoadBalancer;
-import io.grpc.internal.AutoConfiguredLoadBalancerFactory.PolicySelection;
+import io.grpc.internal.ServiceConfigUtil.PolicySelection;
 import io.grpc.util.ForwardingLoadBalancerHelper;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -620,10 +619,6 @@ public void parseLoadBalancerConfig_someProvidesAreNotAvailable() throws Excepti
     assertThat(parsed).isNotNull();
     assertThat(parsed.getConfig()).isNotNull();
     assertThat(((PolicySelection) parsed.getConfig()).config).isNotNull();
-    verify(channelLogger).log(
-        eq(ChannelLogLevel.DEBUG),
-        eq("{0} specified by Service Config are not available"),
-        eq(new ArrayList<>(Collections.singletonList("magic_balancer"))));
   }
 
   @Test

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

@@ -102,10 +102,10 @@
 import io.grpc.Status;
 import io.grpc.Status.Code;
 import io.grpc.StringMarshaller;
-import io.grpc.internal.AutoConfiguredLoadBalancerFactory.PolicySelection;
 import io.grpc.internal.ClientTransportFactory.ClientTransportOptions;
 import io.grpc.internal.InternalSubchannel.TransportLogger;
 import io.grpc.internal.ManagedChannelImpl.ScParser;
+import io.grpc.internal.ServiceConfigUtil.PolicySelection;
 import io.grpc.internal.TestUtils.MockClientTransportInfo;
 import io.grpc.stub.ClientCalls;
 import io.grpc.testing.TestMethodDescriptors;

xds/src/main/java/io/grpc/xds/XdsRoutingLoadBalancerProvider.java

@@ -30,15 +30,14 @@
 import io.grpc.internal.JsonUtil;
 import io.grpc.internal.ServiceConfigUtil;
 import io.grpc.internal.ServiceConfigUtil.LbConfig;
+import io.grpc.internal.ServiceConfigUtil.PolicySelection;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 /**
@@ -50,8 +49,6 @@
 public final class XdsRoutingLoadBalancerProvider extends LoadBalancerProvider {
 
   static final String XDS_ROUTING_POLICY_NAME = "xds_routing_experimental";
-  private static final Logger logger =
-      Logger.getLogger(XdsRoutingLoadBalancerProvider.class.getName());
 
   @Nullable
   private final LoadBalancerRegistry lbRegistry;
@@ -100,7 +97,7 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
         return ConfigOrError.fromError(Status.INTERNAL.withDescription(
             "No actions provided for xds_routing LB policy: " + rawConfig));
       }
-      Map<String, ChildConfig> parsedActions = new LinkedHashMap<>();
+      Map<String, PolicySelection> parsedActions = new LinkedHashMap<>();
       for (String name : actions.keySet()) {
         Map<String, ?> rawAction = JsonUtil.getObject(actions, name);
         if (rawAction == null) {
@@ -114,36 +111,14 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
               "No child policy for action " + name + " in xds_routing LB policy: "
                   + rawConfig));
         }
-        boolean targetParsingSucceeded = false;
-        for (LbConfig lbConfig : childConfigCandidates) {
-          String policyName = lbConfig.getPolicyName();
-          LoadBalancerProvider lbProvider = loadBalancerRegistry().getProvider(policyName);
-          if (lbProvider == null) {
-            logger.log(
-                Level.FINEST,
-                "The policy for {0} is not available in xds_routing LB policy: {1}",
-                new Object[]{policyName, rawConfig});
-          } else {
-            ConfigOrError parsedLbPolicyConfig = lbProvider
-                .parseLoadBalancingPolicyConfig(lbConfig.getRawConfigValue());
-            if (parsedLbPolicyConfig.getError() != null) {
-              // Based on service config error-handling spec, if the chosen config is found invalid
-              // while other configs that come later were valid, the gRPC config would still be
-              // considered invalid as a whole.
-              return parsedLbPolicyConfig;
-            }
-            parsedActions.put(
-                name,
-                new ChildConfig(policyName, parsedLbPolicyConfig.getConfig()));
-            targetParsingSucceeded = true;
-            break;
-          }
-        }
-        if (!targetParsingSucceeded) {
-          return ConfigOrError.fromError(Status.INTERNAL.withDescription(
-              "No child policy available for action " + name + " in xds_routing LB policy: "
-                  + rawConfig));
+
+        ConfigOrError selectedConfigOrError =
+            ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, loadBalancerRegistry());
+        if (selectedConfigOrError.getError() != null) {
+          return selectedConfigOrError;
         }
+
+        parsedActions.put(name, (PolicySelection) selectedConfigOrError.getConfig());
       }
 
       List<Map<String, ?>> routes = JsonUtil.getListOfObjects(rawConfig, "route");
@@ -204,15 +179,14 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
   static final class XdsRoutingConfig {
 
     final List<Route> routes;
-    final Map<String, ChildConfig> actions;
+    final Map<String, PolicySelection> actions;
 
     /**
      * Constructs a deeply parsed xds_routing config with the given non-empty list of routes, the
      * action of each of which is provided by the given map of actions.
      */
     @VisibleForTesting
-    XdsRoutingConfig(List<Route> routes,
-        Map<String, ChildConfig> actions) {
+    XdsRoutingConfig(List<Route> routes, Map<String, PolicySelection> actions) {
       this.routes = ImmutableList.copyOf(routes);
       this.actions = ImmutableMap.copyOf(actions);
     }
@@ -323,42 +297,4 @@ public String toString() {
           .toString();
     }
   }
-
-  static final class ChildConfig {
-
-    final String policyName;
-    final Object config; // Parsed config.
-
-    @VisibleForTesting
-    ChildConfig(String policyName, Object config) {
-      this.policyName = policyName;
-      this.config = config;
-    }
-
-    @Override
-    public boolean equals(Object o) {
-      if (this == o) {
-        return true;
-      }
-      if (o == null || getClass() != o.getClass()) {
-        return false;
-      }
-      ChildConfig that = (ChildConfig) o;
-      return Objects.equals(policyName, that.policyName)
-          && Objects.equals(config, that.config);
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hash(policyName, config);
-    }
-
-    @Override
-    public String toString() {
-      return MoreObjects.toStringHelper(this)
-          .add("policyName", policyName)
-          .add("config", config)
-          .toString();
-    }
-  }
 }