Reviewer comments

src/core/ext/filters/client_channel/client_channel.cc

@@ -762,11 +762,13 @@ class ChannelData::ConnectivityStateAndPickerSetter {
 class ChannelData::ServiceConfigSetter {
  public:
   ServiceConfigSetter(
-      ChannelData* chand,
-      RefCountedPtr<ServerRetryThrottleData> retry_throttle_data,
+      ChannelData* chand, UniquePtr<char> server_name,
+      Optional<internal::ClientChannelGlobalParsedObject::RetryThrottling>
+          retry_throttle_data,
       RefCountedPtr<ServiceConfig> service_config)
       : chand_(chand),
-        retry_throttle_data_(std::move(retry_throttle_data)),
+        server_name_(std::move(server_name)),
+        retry_throttle_data_(retry_throttle_data),
         service_config_(std::move(service_config)) {
     GRPC_CHANNEL_STACK_REF(chand->owning_stack_, "ServiceConfigSetter");
     GRPC_CLOSURE_INIT(&closure_, SetServiceConfigData, this,
@@ -780,7 +782,13 @@ class ChannelData::ServiceConfigSetter {
     ChannelData* chand = self->chand_;
     // Update channel state.
     chand->received_service_config_data_ = true;
-    chand->retry_throttle_data_ = std::move(self->retry_throttle_data_);
+    if (self->retry_throttle_data_.has_value()) {
+      chand->retry_throttle_data_ =
+          internal::ServerRetryThrottleMap::GetDataForServer(
+              self->server_name_.get(),
+              self->retry_throttle_data_.value().max_milli_tokens,
+              self->retry_throttle_data_.value().milli_token_ratio);
+    }
     chand->service_config_ = std::move(self->service_config_);
     // Apply service config to queued picks.
     for (QueuedPick* pick = chand->queued_picks_; pick != nullptr;
@@ -795,7 +803,9 @@ class ChannelData::ServiceConfigSetter {
   }
 
   ChannelData* chand_;
-  RefCountedPtr<ServerRetryThrottleData> retry_throttle_data_;
+  UniquePtr<char> server_name_;
+  Optional<internal::ClientChannelGlobalParsedObject::RetryThrottling>
+      retry_throttle_data_;
   RefCountedPtr<ServiceConfig> service_config_;
   grpc_closure closure_;
 };
@@ -1120,15 +1130,16 @@ bool ChannelData::ProcessResolverResultLocked(
     RefCountedPtr<ParsedLoadBalancingConfig>* lb_policy_config) {
   ChannelData* chand = static_cast<ChannelData*>(arg);
   ProcessedResolverResult resolver_result(result);
-  const char* service_config_json = resolver_result.service_config_json();
+  char* service_config_json = gpr_strdup(resolver_result.service_config_json());
   if (grpc_client_channel_routing_trace.enabled()) {
     gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"",
             chand, service_config_json);
   }
   // Create service config setter to update channel state in the data
   // plane combiner.  Destroys itself when done.
-  New<ServiceConfigSetter>(chand, resolver_result.retry_throttle_data(),
-                           resolver_result.service_config());
+  New<ServiceConfigSetter>(
+      chand, UniquePtr<char>(gpr_strdup(resolver_result.server_name())),
+      resolver_result.retry_throttle_data(), resolver_result.service_config());
   // Swap out the data used by GetChannelInfo().
   bool service_config_changed;
   {
@@ -1140,7 +1151,7 @@ bool ChannelData::ProcessResolverResultLocked(
         (service_config_json != nullptr &&
          strcmp(service_config_json, chand->info_service_config_json_.get()) !=
              0);
-    chand->info_service_config_json_.reset(gpr_strdup(service_config_json));
+    chand->info_service_config_json_.reset(service_config_json);
   }
   // Return results.
   *lb_policy_name = chand->info_lb_policy_name_.get();

src/core/ext/filters/client_channel/client_channel_factory.h

@@ -23,7 +23,6 @@
 
 #include <grpc/impl/codegen/grpc_types.h>
 
-#include "src/core/ext/filters/client_channel/health/health_check_parser.h"
 #include "src/core/ext/filters/client_channel/subchannel.h"
 #include "src/core/lib/gprpp/abstract.h"
 

src/core/ext/filters/client_channel/health/health_check_parser.cc

@@ -25,12 +25,14 @@ namespace {
 size_t g_health_check_parser_index;
 }
 
-UniquePtr<ServiceConfigParsedObject> HealthCheckParser::ParseGlobalParams(
+UniquePtr<ServiceConfig::ParsedConfig> HealthCheckParser::ParseGlobalParams(
     const grpc_json* json, grpc_error** error) {
   GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
   const char* service_name = nullptr;
+  InlinedVector<grpc_error*, 2> error_list;
   for (grpc_json* field = json->child; field != nullptr; field = field->next) {
     if (field->key == nullptr) {
+      GPR_DEBUG_ASSERT(false);
       continue;
     }
     if (strcmp(field->key, "healthCheckConfig") == 0) {
@@ -42,33 +44,39 @@ UniquePtr<ServiceConfigParsedObject> HealthCheckParser::ParseGlobalParams(
       for (grpc_json* sub_field = field->child; sub_field != nullptr;
            sub_field = sub_field->next) {
         if (sub_field->key == nullptr) {
+          GPR_DEBUG_ASSERT(false);
           continue;
         }
         if (strcmp(sub_field->key, "serviceName") == 0) {
           if (service_name != nullptr) {
-            *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                "field:healthCheckConfig field:serviceName error:Duplicate "
-                "entry");
-            return nullptr;
+            error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                "field:serviceName error:Duplicate "
+                "entry"));
+            continue;
           }
           if (sub_field->type != GRPC_JSON_STRING) {
-            *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                "field:healthCheckConfig error:should be of type string");
-            return nullptr;
+            error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+                "field:serviceName error:should be of type string"));
+            continue;
           }
           service_name = sub_field->value;
         }
       }
     }
   }
-  if (service_name == nullptr) return nullptr;
-  return UniquePtr<ServiceConfigParsedObject>(
-      New<HealthCheckParsedObject>(service_name));
+  if (error_list.empty()) {
+    if (service_name == nullptr) return nullptr;
+    return UniquePtr<ServiceConfig::ParsedConfig>(
+        New<HealthCheckParsedObject>(service_name));
+  }
+  *error = ServiceConfig::CreateErrorFromVector("field:healthCheckConfig",
+                                                &error_list);
+  return nullptr;
 }
 
 void HealthCheckParser::Register() {
   g_health_check_parser_index = ServiceConfig::RegisterParser(
-      UniquePtr<ServiceConfigParser>(New<HealthCheckParser>()));
+      UniquePtr<ServiceConfig::Parser>(New<HealthCheckParser>()));
 }
 
 size_t HealthCheckParser::ParserIndex() { return g_health_check_parser_index; }

src/core/ext/filters/client_channel/health/health_check_parser.h

@@ -25,20 +25,22 @@
 
 namespace grpc_core {
 
-class HealthCheckParsedObject : public ServiceConfigParsedObject {
+class HealthCheckParsedObject : public ServiceConfig::ParsedConfig {
  public:
   HealthCheckParsedObject(const char* service_name)
       : service_name_(service_name) {}
 
+  // Returns the service_name found in the health check config. The lifetime of
+  // the string is tied to the lifetime of the ServiceConfig object.
   const char* service_name() const { return service_name_; }
 
  private:
   const char* service_name_;
 };
 
-class HealthCheckParser : public ServiceConfigParser {
+class HealthCheckParser : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(
       const grpc_json* json, grpc_error** error) override;
 
   static void Register();

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

@@ -393,7 +393,7 @@ class GrpcLb : public LoadBalancingPolicy {
   // until it reports READY, at which point it will be moved to child_policy_.
   OrphanablePtr<LoadBalancingPolicy> pending_child_policy_;
   // The child policy config.
-  RefCountedPtr<ParsedLoadBalancingConfig> child_policy_config_ = nullptr;
+  RefCountedPtr<ParsedLoadBalancingConfig> child_policy_config_;
   // Child policy in state READY.
   bool child_policy_ready_ = false;
 };
@@ -1812,8 +1812,7 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory {
           New<ParsedGrpcLbConfig>(nullptr));
     }
     InlinedVector<grpc_error*, 2> error_list;
-    RefCountedPtr<ParsedLoadBalancingConfig> child_policy = nullptr;
-
+    RefCountedPtr<ParsedLoadBalancingConfig> child_policy;
     for (const grpc_json* field = json->child; field != nullptr;
          field = field->next) {
       if (field->key == nullptr) continue;

src/core/ext/filters/client_channel/lb_policy/xds/xds.cc

@@ -1668,11 +1668,11 @@ class XdsFactory : public LoadBalancingPolicyFactory {
     GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
     if (json == nullptr) {
       // xds was mentioned as a policy in the deprecated loadBalancingPolicy
-      // field.
+      // field or in the client API.
       *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
           "field:loadBalancingPolicy error:Xds Parser has required field - "
-          "balancerName. Please use loadBalancingConfig instead of the "
-          "deprecated loadBalancingPolicy");
+          "balancerName. Please use loadBalancingConfig field of service "
+          "config instead.");
       return nullptr;
     }
     GPR_DEBUG_ASSERT(strcmp(json->key, name()) == 0);

src/core/ext/filters/client_channel/lb_policy_registry.cc

@@ -189,15 +189,11 @@ LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(const grpc_json* json,
   }
 }
 
-grpc_error* LoadBalancingPolicyRegistry::ParseDeprecatedLoadBalancingPolicy(
-    const grpc_json* json) {
+grpc_error* LoadBalancingPolicyRegistry::CanCreateLoadBalancingPolicy(
+    const char* lb_policy_name) {
   GPR_DEBUG_ASSERT(g_state != nullptr);
-  GPR_DEBUG_ASSERT(json != nullptr);
-  if (json->type != GRPC_JSON_STRING) {
-    return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-        "field:loadBalancingPolicy error:type should be string");
-  }
-  auto* factory = g_state->GetLoadBalancingPolicyFactory(json->value);
+  gpr_log(GPR_ERROR, "%s", lb_policy_name);
+  auto* factory = g_state->GetLoadBalancingPolicyFactory(lb_policy_name);
   if (factory == nullptr) {
     return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
         "field:loadBalancingPolicy error:Unknown lb policy");

src/core/ext/filters/client_channel/lb_policy_registry.h

@@ -53,13 +53,12 @@ class LoadBalancingPolicyRegistry {
   static bool LoadBalancingPolicyExists(const char* name);
 
   /// Returns a parsed object of the load balancing policy to be used from a
-  /// LoadBalancingConfig array \a json. \a field_name specifies
+  /// LoadBalancingConfig array \a json.
   static RefCountedPtr<ParsedLoadBalancingConfig> ParseLoadBalancingConfig(
       const grpc_json* json, grpc_error** error);
 
-  /// Validates if the deprecated loadBalancingPolicy field can be parsed.
-  /// Returns GRPC_ERROR_NONE if successfully parsed.
-  static grpc_error* ParseDeprecatedLoadBalancingPolicy(const grpc_json* json);
+  /// Validates if a load balancing policy can be created from \a lb_policy_name
+  static grpc_error* CanCreateLoadBalancingPolicy(const char* lb_policy_name);
 };
 
 }  // namespace grpc_core

src/core/ext/filters/client_channel/resolver_result_parsing.cc

@@ -29,6 +29,7 @@
 #include <grpc/support/string_util.h>
 
 #include "src/core/ext/filters/client_channel/client_channel.h"
+#include "src/core/ext/filters/client_channel/health/health_check_parser.h"
 #include "src/core/ext/filters/client_channel/lb_policy_registry.h"
 #include "src/core/ext/filters/client_channel/server_address.h"
 #include "src/core/lib/channel/channel_args.h"
@@ -53,8 +54,9 @@ size_t ClientChannelServiceConfigParser::ParserIndex() {
 }
 
 void ClientChannelServiceConfigParser::Register() {
-  g_client_channel_service_config_parser_index = ServiceConfig::RegisterParser(
-      UniquePtr<ServiceConfigParser>(New<ClientChannelServiceConfigParser>()));
+  g_client_channel_service_config_parser_index =
+      ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser>(
+          New<ClientChannelServiceConfigParser>()));
 }
 
 ProcessedResolverResult::ProcessedResolverResult(
@@ -73,60 +75,57 @@ ProcessedResolverResult::ProcessedResolverResult(
     }
   }
   // Process service config.
-  ProcessServiceConfig(resolver_result);
-  ProcessLbPolicy(resolver_result);
+  const ClientChannelGlobalParsedObject* parsed_object = nullptr;
+  if (service_config_ != nullptr) {
+    parsed_object = static_cast<const ClientChannelGlobalParsedObject*>(
+        service_config_->GetParsedGlobalServiceConfigObject(
+            ClientChannelServiceConfigParser::ParserIndex()));
+    ProcessServiceConfig(resolver_result, parsed_object);
+  }
+  ProcessLbPolicy(resolver_result, parsed_object);
 }
 
 void ProcessedResolverResult::ProcessServiceConfig(
-    const Resolver::Result& resolver_result) {
-  if (service_config_ == nullptr) return;
+    const Resolver::Result& resolver_result,
+    const ClientChannelGlobalParsedObject* parsed_object) {
   auto* health_check = static_cast<HealthCheckParsedObject*>(
       service_config_->GetParsedGlobalServiceConfigObject(
           HealthCheckParser::ParserIndex()));
   health_check_service_name_ =
       health_check != nullptr ? health_check->service_name() : nullptr;
   service_config_json_ = service_config_->service_config_json();
-  auto* parsed_object = static_cast<const ClientChannelGlobalParsedObject*>(
-      service_config_->GetParsedGlobalServiceConfigObject(
-          ClientChannelServiceConfigParser::ParserIndex()));
   if (!parsed_object) {
     return;
   }
-  const grpc_arg* channel_arg =
-      grpc_channel_args_find(resolver_result.args, GRPC_ARG_SERVER_URI);
-  const char* server_uri = grpc_channel_arg_get_string(channel_arg);
-  GPR_ASSERT(server_uri != nullptr);
-  grpc_uri* uri = grpc_uri_parse(server_uri, true);
-  GPR_ASSERT(uri->path[0] != '\0');
   if (parsed_object->retry_throttling().has_value()) {
-    char* server_name = uri->path[0] == '/' ? uri->path + 1 : uri->path;
-    retry_throttle_data_ = internal::ServerRetryThrottleMap::GetDataForServer(
-        server_name, parsed_object->retry_throttling().value().max_milli_tokens,
-        parsed_object->retry_throttling().value().milli_token_ratio);
+    const grpc_arg* channel_arg =
+        grpc_channel_args_find(resolver_result.args, GRPC_ARG_SERVER_URI);
+    const char* server_uri = grpc_channel_arg_get_string(channel_arg);
+    GPR_ASSERT(server_uri != nullptr);
+    grpc_uri* uri = grpc_uri_parse(server_uri, true);
+    GPR_ASSERT(uri->path[0] != '\0');
+    server_name_ = uri->path[0] == '/' ? uri->path + 1 : uri->path;
+    retry_throttle_data_ = parsed_object->retry_throttling();
+    grpc_uri_destroy(uri);
   }
-  grpc_uri_destroy(uri);
 }
 
 void ProcessedResolverResult::ProcessLbPolicy(
-    const Resolver::Result& resolver_result) {
+    const Resolver::Result& resolver_result,
+    const ClientChannelGlobalParsedObject* parsed_object) {
   // Prefer the LB policy name found in the service config.
-  if (service_config_ != nullptr) {
-    auto* parsed_object = static_cast<const ClientChannelGlobalParsedObject*>(
-        service_config_->GetParsedGlobalServiceConfigObject(
-            ClientChannelServiceConfigParser::ParserIndex()));
-    if (parsed_object != nullptr) {
-      if (parsed_object->parsed_lb_config() != nullptr) {
-        lb_policy_name_.reset(
-            gpr_strdup(parsed_object->parsed_lb_config()->name()));
-        lb_policy_config_ = parsed_object->parsed_lb_config();
-      } else {
-        lb_policy_name_.reset(
-            gpr_strdup(parsed_object->parsed_deprecated_lb_policy()));
-        if (lb_policy_name_ != nullptr) {
-          char* lb_policy_name = lb_policy_name_.get();
-          for (size_t i = 0; i < strlen(lb_policy_name); ++i) {
-            lb_policy_name[i] = tolower(lb_policy_name[i]);
-          }
+  if (parsed_object != nullptr) {
+    if (parsed_object->parsed_lb_config() != nullptr) {
+      lb_policy_name_.reset(
+          gpr_strdup(parsed_object->parsed_lb_config()->name()));
+      lb_policy_config_ = parsed_object->parsed_lb_config();
+    } else {
+      lb_policy_name_.reset(
+          gpr_strdup(parsed_object->parsed_deprecated_lb_policy()));
+      if (lb_policy_name_ != nullptr) {
+        char* lb_policy_name = lb_policy_name_.get();
+        for (size_t i = 0; i < strlen(lb_policy_name); ++i) {
+          lb_policy_name[i] = tolower(lb_policy_name[i]);
         }
       }
     }
@@ -226,7 +225,7 @@ UniquePtr<ClientChannelMethodParsedObject::RetryPolicy> ParseRetryPolicy(
       retry_policy->max_attempts = gpr_parse_nonnegative_int(sub_field->value);
       if (retry_policy->max_attempts <= 1) {
         error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-            "field:maxAttempts error:should be atleast 2"));
+            "field:maxAttempts error:should be at least 2"));
         continue;
       }
       if (retry_policy->max_attempts > MAX_MAX_RETRY_ATTEMPTS) {
@@ -336,13 +335,13 @@ UniquePtr<ClientChannelMethodParsedObject::RetryPolicy> ParseRetryPolicy(
 
 }  // namespace
 
-UniquePtr<ServiceConfigParsedObject>
+UniquePtr<ServiceConfig::ParsedConfig>
 ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json,
                                                     grpc_error** error) {
   GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
   InlinedVector<grpc_error*, 4> error_list;
   RefCountedPtr<ParsedLoadBalancingConfig> parsed_lb_config;
-  const char* lb_policy_name = nullptr;
+  UniquePtr<char> lb_policy_name;
   Optional<ClientChannelGlobalParsedObject::RetryThrottling> retry_throttling;
   for (grpc_json* field = json->child; field != nullptr; field = field->next) {
     if (field->key == nullptr) {
@@ -368,21 +367,28 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json,
       if (lb_policy_name != nullptr) {
         error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
             "field:loadBalancingPolicy error:Duplicate entry"));
+        continue;
       } else if (field->type != GRPC_JSON_STRING) {
         error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
             "field:loadBalancingPolicy error:type should be string"));
-      } else if (!LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(
-                     field->value)) {
+        continue;
+      }
+      lb_policy_name.reset(gpr_strdup(field->value));
+      char* lb_policy = lb_policy_name.get();
+      if (lb_policy != nullptr) {
+        for (size_t i = 0; i < strlen(lb_policy); ++i) {
+          lb_policy[i] = tolower(lb_policy[i]);
+        }
+      }
+      if (!LoadBalancingPolicyRegistry::LoadBalancingPolicyExists(lb_policy)) {
         error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
             "field:loadBalancingPolicy error:Unknown lb policy"));
       } else {
         grpc_error* parsing_error =
-            LoadBalancingPolicyRegistry::ParseDeprecatedLoadBalancingPolicy(
-                field);
+            LoadBalancingPolicyRegistry::CanCreateLoadBalancingPolicy(
+                lb_policy);
         if (parsing_error != GRPC_ERROR_NONE) {
           error_list.push_back(parsing_error);
-        } else {
-          lb_policy_name = field->value;
         }
       }
     }
@@ -490,14 +496,15 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json,
   *error = ServiceConfig::CreateErrorFromVector("Client channel global parser",
                                                 &error_list);
   if (*error == GRPC_ERROR_NONE) {
-    return UniquePtr<ServiceConfigParsedObject>(
+    return UniquePtr<ServiceConfig::ParsedConfig>(
         New<ClientChannelGlobalParsedObject>(std::move(parsed_lb_config),
-                                             lb_policy_name, retry_throttling));
+                                             std::move(lb_policy_name),
+                                             retry_throttling));
   }
   return nullptr;
 }
 
-UniquePtr<ServiceConfigParsedObject>
+UniquePtr<ServiceConfig::ParsedConfig>
 ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json,
                                                        grpc_error** error) {
   GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
@@ -547,7 +554,7 @@ ClientChannelServiceConfigParser::ParsePerMethodParams(const grpc_json* json,
   *error = ServiceConfig::CreateErrorFromVector("Client channel parser",
                                                 &error_list);
   if (*error == GRPC_ERROR_NONE) {
-    return UniquePtr<ServiceConfigParsedObject>(
+    return UniquePtr<ServiceConfig::ParsedConfig>(
         New<ClientChannelMethodParsedObject>(timeout, wait_for_ready,
                                              std::move(retry_policy)));
   }

src/core/ext/filters/client_channel/resolver_result_parsing.h

@@ -37,19 +37,19 @@
 namespace grpc_core {
 namespace internal {
 
-class ClientChannelGlobalParsedObject : public ServiceConfigParsedObject {
+class ClientChannelGlobalParsedObject : public ServiceConfig::ParsedConfig {
  public:
   struct RetryThrottling {
-    int max_milli_tokens = 0;
-    int milli_token_ratio = 0;
+    intptr_t max_milli_tokens = 0;
+    intptr_t milli_token_ratio = 0;
   };
 
   ClientChannelGlobalParsedObject(
       RefCountedPtr<ParsedLoadBalancingConfig> parsed_lb_config,
-      const char* parsed_deprecated_lb_policy,
+      UniquePtr<char> parsed_deprecated_lb_policy,
       const Optional<RetryThrottling>& retry_throttling)
       : parsed_lb_config_(std::move(parsed_lb_config)),
-        parsed_deprecated_lb_policy_(parsed_deprecated_lb_policy),
+        parsed_deprecated_lb_policy_(std::move(parsed_deprecated_lb_policy)),
         retry_throttling_(retry_throttling) {}
 
   Optional<RetryThrottling> retry_throttling() const {
@@ -61,16 +61,16 @@ class ClientChannelGlobalParsedObject : public ServiceConfigParsedObject {
   }
 
   const char* parsed_deprecated_lb_policy() const {
-    return parsed_deprecated_lb_policy_;
+    return parsed_deprecated_lb_policy_.get();
   }
 
  private:
   RefCountedPtr<ParsedLoadBalancingConfig> parsed_lb_config_;
-  const char* parsed_deprecated_lb_policy_ = nullptr;
+  UniquePtr<char> parsed_deprecated_lb_policy_;
   Optional<RetryThrottling> retry_throttling_;
 };
 
-class ClientChannelMethodParsedObject : public ServiceConfigParsedObject {
+class ClientChannelMethodParsedObject : public ServiceConfig::ParsedConfig {
  public:
   struct RetryPolicy {
     int max_attempts = 0;
@@ -99,12 +99,12 @@ class ClientChannelMethodParsedObject : public ServiceConfigParsedObject {
   UniquePtr<RetryPolicy> retry_policy_;
 };
 
-class ClientChannelServiceConfigParser : public ServiceConfigParser {
+class ClientChannelServiceConfigParser : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(
       const grpc_json* json, grpc_error** error) override;
 
-  UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParsePerMethodParams(
       const grpc_json* json, grpc_error** error) override;
 
   static size_t ParserIndex();
@@ -121,8 +121,12 @@ class ProcessedResolverResult {
 
   // Getters. Any managed object's ownership is transferred.
   const char* service_config_json() { return service_config_json_; }
-  RefCountedPtr<ServerRetryThrottleData> retry_throttle_data() {
-    return std::move(retry_throttle_data_);
+
+  const char* server_name() { return server_name_; }
+
+  Optional<ClientChannelGlobalParsedObject::RetryThrottling>
+  retry_throttle_data() {
+    return retry_throttle_data_;
   }
 
   UniquePtr<char> lb_policy_name() { return std::move(lb_policy_name_); }
@@ -131,15 +135,19 @@ class ProcessedResolverResult {
   }
 
   const char* health_check_service_name() { return health_check_service_name_; }
+
   RefCountedPtr<ServiceConfig> service_config() { return service_config_; }
 
  private:
   // Finds the service config; extracts LB config and (maybe) retry throttle
   // params from it.
-  void ProcessServiceConfig(const Resolver::Result& resolver_result);
+  void ProcessServiceConfig(
+      const Resolver::Result& resolver_result,
+      const ClientChannelGlobalParsedObject* parsed_object);
 
   // Extracts the LB policy.
-  void ProcessLbPolicy(const Resolver::Result& resolver_result);
+  void ProcessLbPolicy(const Resolver::Result& resolver_result,
+                       const ClientChannelGlobalParsedObject* parsed_object);
 
   // Parses the service config. Intended to be used by
   // ServiceConfig::ParseGlobalParams.
@@ -157,7 +165,9 @@ class ProcessedResolverResult {
   UniquePtr<char> lb_policy_name_;
   RefCountedPtr<ParsedLoadBalancingConfig> lb_policy_config_ = nullptr;
   // Retry throttle data.
-  RefCountedPtr<ServerRetryThrottleData> retry_throttle_data_;
+  const char* server_name_ = nullptr;
+  Optional<ClientChannelGlobalParsedObject::RetryThrottling>
+      retry_throttle_data_;
   const char* health_check_service_name_ = nullptr;
 };
 

src/core/ext/filters/client_channel/service_config.cc

@@ -34,7 +34,7 @@
 namespace grpc_core {
 
 namespace {
-typedef InlinedVector<UniquePtr<ServiceConfigParser>,
+typedef InlinedVector<UniquePtr<ServiceConfig::Parser>,
                       ServiceConfig::kNumPreallocatedParsers>
     ServiceConfigParserList;
 ServiceConfigParserList* g_registered_parsers;
@@ -78,7 +78,6 @@ ServiceConfig::ServiceConfig(UniquePtr<char> service_config_json,
     error_list[error_count++] = local_error;
   }
   if (error_count > 0) {
-    gpr_log(GPR_ERROR, "got errors");
     *error = GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
         "Service config parsing error", error_list, error_count);
     GRPC_ERROR_UNREF(global_error);
@@ -314,7 +313,7 @@ ServiceConfig::GetMethodServiceConfigObjectsVector(const grpc_slice& path) {
   return *value;
 }
 
-size_t ServiceConfig::RegisterParser(UniquePtr<ServiceConfigParser> parser) {
+size_t ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser> parser) {
   g_registered_parsers->push_back(std::move(parser));
   return g_registered_parsers->size() - 1;
 }

src/core/ext/filters/client_channel/service_config.h

@@ -55,39 +55,39 @@
 
 namespace grpc_core {
 
-/// This is the base class that all service config parsers MUST use to store
-/// parsed service config data.
-class ServiceConfigParsedObject {
+class ServiceConfig : public RefCounted<ServiceConfig> {
  public:
-  virtual ~ServiceConfigParsedObject() = default;
+  /// This is the base class that all service config parsers MUST use to store
+  /// parsed service config data.
+  class ParsedConfig {
+   public:
+    virtual ~ParsedConfig() = default;
 
-  GRPC_ABSTRACT_BASE_CLASS;
-};
+    GRPC_ABSTRACT_BASE_CLASS;
+  };
 
-/// This is the base class that all service config parsers should derive from.
-class ServiceConfigParser {
- public:
-  virtual ~ServiceConfigParser() = default;
+  /// This is the base class that all service config parsers should derive from.
+  class Parser {
+   public:
+    virtual ~Parser() = default;
 
-  virtual UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
-      const grpc_json* json, grpc_error** error) {
-    GPR_DEBUG_ASSERT(error != nullptr);
-    return nullptr;
-  }
+    virtual UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(
+        const grpc_json* json, grpc_error** error) {
+      GPR_DEBUG_ASSERT(error != nullptr);
+      return nullptr;
+    }
 
-  virtual UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
-      const grpc_json* json, grpc_error** error) {
-    GPR_DEBUG_ASSERT(error != nullptr);
-    return nullptr;
-  }
+    virtual UniquePtr<ServiceConfig::ParsedConfig> ParsePerMethodParams(
+        const grpc_json* json, grpc_error** error) {
+      GPR_DEBUG_ASSERT(error != nullptr);
+      return nullptr;
+    }
 
-  GRPC_ABSTRACT_BASE_CLASS;
-};
+    GRPC_ABSTRACT_BASE_CLASS;
+  };
 
-class ServiceConfig : public RefCounted<ServiceConfig> {
- public:
   static constexpr int kNumPreallocatedParsers = 4;
-  typedef InlinedVector<UniquePtr<ServiceConfigParsedObject>,
+  typedef InlinedVector<UniquePtr<ServiceConfig::ParsedConfig>,
                         kNumPreallocatedParsers>
       ServiceConfigObjectsVector;
 
@@ -104,7 +104,7 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
 
     RefCountedPtr<ServiceConfig> service_config() { return service_config_; }
 
-    ServiceConfigParsedObject* GetMethodParsedObject(int index) const {
+    ServiceConfig::ParsedConfig* GetMethodParsedObject(int index) const {
       return method_params_vector_ != nullptr
                  ? (*method_params_vector_)[index].get()
                  : nullptr;
@@ -127,14 +127,18 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
 
   const char* service_config_json() const { return service_config_json_.get(); }
 
-  /// Retrieves the parsed global service config object at index \a index.
-  ServiceConfigParsedObject* GetParsedGlobalServiceConfigObject(size_t index) {
+  /// Retrieves the parsed global service config object at index \a index. The
+  /// lifetime of the returned object is tied to the lifetime of the
+  /// ServiceConfig object.
+  ServiceConfig::ParsedConfig* GetParsedGlobalServiceConfigObject(
+      size_t index) {
     GPR_DEBUG_ASSERT(index < parsed_global_service_config_objects_.size());
     return parsed_global_service_config_objects_[index].get();
   }
 
   /// Retrieves the vector of method service config objects for a given path \a
-  /// path.
+  /// path. The lifetime of the returned vector and contained objects is tied to
+  /// the lifetime of the ServiceConfig object.
   const ServiceConfigObjectsVector* GetMethodServiceConfigObjectsVector(
       const grpc_slice& path);
 
@@ -144,7 +148,7 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
   /// registered parser. Each parser is responsible for reading the service
   /// config json and returning a parsed object. This parsed object can later be
   /// retrieved using the same index that was returned at registration time.
-  static size_t RegisterParser(UniquePtr<ServiceConfigParser> parser);
+  static size_t RegisterParser(UniquePtr<ServiceConfig::Parser> parser);
 
   static void Init();
 
@@ -199,7 +203,7 @@ class ServiceConfig : public RefCounted<ServiceConfig> {
   UniquePtr<char> json_string_;  // Underlying storage for json_tree.
   grpc_json* json_tree_;
 
-  InlinedVector<UniquePtr<ServiceConfigParsedObject>, kNumPreallocatedParsers>
+  InlinedVector<UniquePtr<ServiceConfig::ParsedConfig>, kNumPreallocatedParsers>
       parsed_global_service_config_objects_;
   // A map from the method name to the service config objects vector. Note that
   // we are using a raw pointer and not a unique pointer so that we can use the

src/core/ext/filters/client_channel/subchannel.h

@@ -23,7 +23,6 @@
 
 #include "src/core/ext/filters/client_channel/client_channel_channelz.h"
 #include "src/core/ext/filters/client_channel/connector.h"
-#include "src/core/ext/filters/client_channel/health/health_check_parser.h"
 #include "src/core/ext/filters/client_channel/subchannel_pool_interface.h"
 #include "src/core/lib/backoff/backoff.h"
 #include "src/core/lib/channel/channel_stack.h"

src/core/ext/filters/message_size/message_size_filter.cc

@@ -44,7 +44,7 @@ size_t g_message_size_parser_index;
 
 namespace grpc_core {
 
-UniquePtr<ServiceConfigParsedObject> MessageSizeParser::ParsePerMethodParams(
+UniquePtr<ServiceConfig::ParsedConfig> MessageSizeParser::ParsePerMethodParams(
     const grpc_json* json, grpc_error** error) {
   GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE);
   int max_request_message_bytes = -1;
@@ -89,13 +89,13 @@ UniquePtr<ServiceConfigParsedObject> MessageSizeParser::ParsePerMethodParams(
                                                   &error_list);
     return nullptr;
   }
-  return UniquePtr<ServiceConfigParsedObject>(New<MessageSizeParsedObject>(
+  return UniquePtr<ServiceConfig::ParsedConfig>(New<MessageSizeParsedObject>(
       max_request_message_bytes, max_response_message_bytes));
 }
 
 void MessageSizeParser::Register() {
   g_message_size_parser_index = ServiceConfig::RegisterParser(
-      UniquePtr<ServiceConfigParser>(New<MessageSizeParser>()));
+      UniquePtr<ServiceConfig::Parser>(New<MessageSizeParser>()));
 }
 
 size_t MessageSizeParser::ParserIndex() { return g_message_size_parser_index; }

src/core/ext/filters/message_size/message_size_filter.h

@@ -26,7 +26,7 @@ extern const grpc_channel_filter grpc_message_size_filter;
 
 namespace grpc_core {
 
-class MessageSizeParsedObject : public ServiceConfigParsedObject {
+class MessageSizeParsedObject : public ServiceConfig::ParsedConfig {
  public:
   struct message_size_limits {
     int max_send_size;
@@ -44,9 +44,9 @@ class MessageSizeParsedObject : public ServiceConfigParsedObject {
   message_size_limits limits_;
 };
 
-class MessageSizeParser : public ServiceConfigParser {
+class MessageSizeParser : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParsePerMethodParams(
       const grpc_json* json, grpc_error** error) override;
 
   static void Register();

test/core/client_channel/service_config_test.cc

@@ -24,7 +24,7 @@
 #include "src/core/ext/filters/client_channel/health/health_check_parser.h"
 #include "src/core/ext/filters/client_channel/resolver_result_parsing.h"
 #include "src/core/ext/filters/client_channel/service_config.h"
-#include "src/core/ext/filters/message_size/message_size_parser.h"
+#include "src/core/ext/filters/message_size/message_size_filter.h"
 #include "src/core/lib/gpr/string.h"
 #include "test/core/util/port.h"
 #include "test/core/util/test_config.h"
@@ -32,7 +32,7 @@
 namespace grpc_core {
 namespace testing {
 
-class TestParsedObject1 : public ServiceConfigParsedObject {
+class TestParsedObject1 : public ServiceConfig::ParsedConfig {
  public:
   TestParsedObject1(int value) : value_(value) {}
 
@@ -42,9 +42,9 @@ class TestParsedObject1 : public ServiceConfigParsedObject {
   int value_;
 };
 
-class TestParser1 : public ServiceConfigParser {
+class TestParser1 : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(
       const grpc_json* json, grpc_error** error) override {
     GPR_DEBUG_ASSERT(error != nullptr);
     for (grpc_json* field = json->child; field != nullptr;
@@ -61,7 +61,7 @@ class TestParser1 : public ServiceConfigParser {
               GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage());
           return nullptr;
         }
-        return UniquePtr<ServiceConfigParsedObject>(
+        return UniquePtr<ServiceConfig::ParsedConfig>(
             New<TestParsedObject1>(value));
       }
     }
@@ -77,9 +77,9 @@ class TestParser1 : public ServiceConfigParser {
   }
 };
 
-class TestParser2 : public ServiceConfigParser {
+class TestParser2 : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParsePerMethodParams(
       const grpc_json* json, grpc_error** error) override {
     GPR_DEBUG_ASSERT(error != nullptr);
     for (grpc_json* field = json->child; field != nullptr;
@@ -99,7 +99,7 @@ class TestParser2 : public ServiceConfigParser {
               GRPC_ERROR_CREATE_FROM_STATIC_STRING(InvalidValueErrorMessage());
           return nullptr;
         }
-        return UniquePtr<ServiceConfigParsedObject>(
+        return UniquePtr<ServiceConfig::ParsedConfig>(
             New<TestParsedObject1>(value));
       }
     }
@@ -116,16 +116,16 @@ class TestParser2 : public ServiceConfigParser {
 };
 
 // This parser always adds errors
-class ErrorParser : public ServiceConfigParser {
+class ErrorParser : public ServiceConfig::Parser {
  public:
-  UniquePtr<ServiceConfigParsedObject> ParsePerMethodParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParsePerMethodParams(
       const grpc_json* json, grpc_error** error) override {
     GPR_DEBUG_ASSERT(error != nullptr);
     *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(MethodError());
     return nullptr;
   }
 
-  UniquePtr<ServiceConfigParsedObject> ParseGlobalParams(
+  UniquePtr<ServiceConfig::ParsedConfig> ParseGlobalParams(
       const grpc_json* json, grpc_error** error) override {
     GPR_DEBUG_ASSERT(error != nullptr);
     *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(GlobalError());
@@ -137,15 +137,22 @@ class ErrorParser : public ServiceConfigParser {
   static const char* GlobalError() { return "ErrorParser : globalError"; }
 };
 
+void VerifyRegexMatch(grpc_error* error, std::regex e) {
+  std::smatch match;
+  std::string s(grpc_error_string(error));
+  EXPECT_TRUE(std::regex_search(s, match, e));
+  GRPC_ERROR_UNREF(error);
+}
+
 class ServiceConfigTest : public ::testing::Test {
  protected:
   void SetUp() override {
     ServiceConfig::Shutdown();
     ServiceConfig::Init();
     EXPECT_TRUE(ServiceConfig::RegisterParser(
-                    UniquePtr<ServiceConfigParser>(New<TestParser1>())) == 0);
+                    UniquePtr<ServiceConfig::Parser>(New<TestParser1>())) == 0);
     EXPECT_TRUE(ServiceConfig::RegisterParser(
-                    UniquePtr<ServiceConfigParser>(New<TestParser2>())) == 1);
+                    UniquePtr<ServiceConfig::Parser>(New<TestParser2>())) == 1);
   }
 };
 
@@ -156,10 +163,7 @@ TEST_F(ServiceConfigTest, ErrorCheck1) {
   gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
   ASSERT_TRUE(error != GRPC_ERROR_NONE);
   std::regex e(std::string("failed to parse JSON for service config"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, BasicTest1) {
@@ -181,10 +185,7 @@ TEST_F(ServiceConfigTest, ErrorNoNames) {
                   "Params)(.*)(referenced_errors)(.*)(No names "
                   "found)(.*)(methodConfig)(.*)(referenced_errors)(.*)(No "
                   "names specified)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) {
@@ -200,10 +201,7 @@ TEST_F(ServiceConfigTest, ErrorNoNamesWithMultipleMethodConfigs) {
                   "Params)(.*)(referenced_errors)(.*)(No names "
                   "found)(.*)(methodConfig)(.*)(referenced_errors)(.*)(No "
                   "names specified)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, ValidMethodConfig) {
@@ -247,10 +245,7 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidType) {
                            "error)(.*)(referenced_errors)(.*)(Global "
                            "Params)(.*)(referenced_errors)(.*)") +
                TestParser1::InvalidTypeErrorMessage());
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) {
@@ -263,10 +258,7 @@ TEST_F(ServiceConfigTest, Parser1ErrorInvalidValue) {
                            "error)(.*)(referenced_errors)(.*)(Global "
                            "Params)(.*)(referenced_errors)(.*)") +
                TestParser1::InvalidValueErrorMessage());
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, Parser2BasicTest) {
@@ -296,10 +288,7 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidType) {
                            "Params)(.*)(referenced_errors)(.*)(methodConfig)("
                            ".*)(referenced_errors)(.*)") +
                TestParser2::InvalidTypeErrorMessage());
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) {
@@ -315,10 +304,7 @@ TEST_F(ServiceConfigTest, Parser2ErrorInvalidValue) {
                            "Params)(.*)(referenced_errors)()(.*)(methodConfig)("
                            ".*)(referenced_errors)(.*)") +
                TestParser2::InvalidValueErrorMessage());
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 // Test parsing with ErrorParsers which always add errors
@@ -328,9 +314,9 @@ class ErroredParsersScopingTest : public ::testing::Test {
     ServiceConfig::Shutdown();
     ServiceConfig::Init();
     EXPECT_TRUE(ServiceConfig::RegisterParser(
-                    UniquePtr<ServiceConfigParser>(New<ErrorParser>())) == 0);
+                    UniquePtr<ServiceConfig::Parser>(New<ErrorParser>())) == 0);
     EXPECT_TRUE(ServiceConfig::RegisterParser(
-                    UniquePtr<ServiceConfigParser>(New<ErrorParser>())) == 1);
+                    UniquePtr<ServiceConfig::Parser>(New<ErrorParser>())) == 1);
   }
 };
 
@@ -345,10 +331,7 @@ TEST_F(ErroredParsersScopingTest, GlobalParams) {
                            "Params)(.*)(referenced_errors)()(.*)") +
                ErrorParser::GlobalError() + std::string("(.*)") +
                ErrorParser::GlobalError());
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ErroredParsersScopingTest, MethodParams) {
@@ -369,10 +352,7 @@ TEST_F(ErroredParsersScopingTest, MethodParams) {
                   "found)(.*)(methodConfig)(.*)(referenced_errors)(.*)") +
       ErrorParser::MethodError() + std::string("(.*)") +
       ErrorParser::MethodError() + std::string("(.*)(No names specified)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 class ClientChannelParserTest : public ::testing::Test {
@@ -381,7 +361,7 @@ class ClientChannelParserTest : public ::testing::Test {
     ServiceConfig::Shutdown();
     ServiceConfig::Init();
     EXPECT_TRUE(
-        ServiceConfig::RegisterParser(UniquePtr<ServiceConfigParser>(
+        ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser>(
             New<grpc_core::internal::ClientChannelServiceConfigParser>())) ==
         0);
   }
@@ -395,7 +375,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigPickFirst) {
   const auto* parsed_object =
       static_cast<grpc_core::internal::ClientChannelGlobalParsedObject*>(
           svc_cfg->GetParsedGlobalServiceConfigObject(0));
-  const auto* lb_config = parsed_object->parsed_lb_config();
+  auto lb_config = parsed_object->parsed_lb_config();
   EXPECT_TRUE(strcmp(lb_config->name(), "pick_first") == 0);
 }
 
@@ -405,10 +385,10 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigRoundRobin) {
   grpc_error* error = GRPC_ERROR_NONE;
   auto svc_cfg = ServiceConfig::Create(test_json, &error);
   ASSERT_TRUE(error == GRPC_ERROR_NONE);
-  const auto* parsed_object =
+  auto parsed_object =
       static_cast<grpc_core::internal::ClientChannelGlobalParsedObject*>(
           svc_cfg->GetParsedGlobalServiceConfigObject(0));
-  const auto* lb_config = parsed_object->parsed_lb_config();
+  auto lb_config = parsed_object->parsed_lb_config();
   EXPECT_TRUE(strcmp(lb_config->name(), "round_robin") == 0);
 }
 
@@ -422,7 +402,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigGrpclb) {
   const auto* parsed_object =
       static_cast<grpc_core::internal::ClientChannelGlobalParsedObject*>(
           svc_cfg->GetParsedGlobalServiceConfigObject(0));
-  const auto* lb_config = parsed_object->parsed_lb_config();
+  auto lb_config = parsed_object->parsed_lb_config();
   EXPECT_TRUE(strcmp(lb_config->name(), "grpclb") == 0);
 }
 
@@ -441,7 +421,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigXds) {
   const auto* parsed_object =
       static_cast<grpc_core::internal::ClientChannelGlobalParsedObject*>(
           svc_cfg->GetParsedGlobalServiceConfigObject(0));
-  const auto* lb_config = parsed_object->parsed_lb_config();
+  auto lb_config = parsed_object->parsed_lb_config();
   EXPECT_TRUE(strcmp(lb_config->name(), "xds_experimental") == 0);
 }
 
@@ -457,13 +437,10 @@ TEST_F(ClientChannelParserTest, UnknownLoadBalancingConfig) {
                   "Params)(.*)(referenced_errors)(.*)(Client channel global "
                   "parser)(.*)(referenced_errors)(.*)(field:"
                   "loadBalancingConfig error:No known policy)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
-TEST_F(ClientChannelParserTest, InvalidgRPCLbLoadBalancingConfig) {
+TEST_F(ClientChannelParserTest, InvalidGrpclbLoadBalancingConfig) {
   const char* test_json =
       "{\"loadBalancingConfig\": "
       "[{\"grpclb\":{\"childPolicy\":[{\"unknown\":{}}]}}]}";
@@ -478,10 +455,7 @@ TEST_F(ClientChannelParserTest, InvalidgRPCLbLoadBalancingConfig) {
                   "parser)(.*)(referenced_errors)(.*)(GrpcLb "
                   "Parser)(.*)(referenced_errors)(.*)(field:childPolicy "
                   "error:No known policy)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InalidLoadBalancingConfigXds) {
@@ -503,10 +477,7 @@ TEST_F(ClientChannelParserTest, InalidLoadBalancingConfigXds) {
                   "parser)(.*)(referenced_errors)(.*)(Xds "
                   "Parser)(.*)(referenced_errors)(.*)(field:balancerName "
                   "error:not found)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, ValidLoadBalancingPolicy) {
@@ -522,6 +493,20 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingPolicy) {
   EXPECT_TRUE(strcmp(lb_policy, "pick_first") == 0);
 }
 
+TEST_F(ClientChannelParserTest, ValidLoadBalancingPolicyAllCaps) {
+  const char* test_json = "{\"loadBalancingPolicy\":\"PICK_FIRST\"}";
+  grpc_error* error = GRPC_ERROR_NONE;
+  auto svc_cfg = ServiceConfig::Create(test_json, &error);
+  gpr_log(GPR_ERROR, "%s", grpc_error_string(error));
+  ASSERT_TRUE(error == GRPC_ERROR_NONE);
+  const auto* parsed_object =
+      static_cast<grpc_core::internal::ClientChannelGlobalParsedObject*>(
+          svc_cfg->GetParsedGlobalServiceConfigObject(0));
+  const auto* lb_policy = parsed_object->parsed_deprecated_lb_policy();
+  ASSERT_TRUE(lb_policy != nullptr);
+  EXPECT_TRUE(strcmp(lb_policy, "pick_first") == 0);
+}
+
 TEST_F(ClientChannelParserTest, UnknownLoadBalancingPolicy) {
   const char* test_json = "{\"loadBalancingPolicy\":\"unknown\"}";
   grpc_error* error = GRPC_ERROR_NONE;
@@ -534,10 +519,7 @@ TEST_F(ClientChannelParserTest, UnknownLoadBalancingPolicy) {
                   "Params)(.*)(referenced_errors)(.*)(Client channel global "
                   "parser)(.*)(referenced_errors)(.*)(field:"
                   "loadBalancingPolicy error:Unknown lb policy)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, LoadBalancingPolicyXdsNotAllowed) {
@@ -552,11 +534,8 @@ TEST_F(ClientChannelParserTest, LoadBalancingPolicyXdsNotAllowed) {
       "Params)(.*)(referenced_errors)(.*)(Client channel global "
       "parser)(.*)(referenced_errors)(.*)(field:loadBalancingPolicy error:Xds "
       "Parser has required field - balancerName. Please use "
-      "loadBalancingConfig instead of the deprecated loadBalancingPolicy)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+      "loadBalancingConfig field of service config instead.)"));
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, ValidRetryThrottling) {
@@ -597,10 +576,7 @@ TEST_F(ClientChannelParserTest, RetryThrottlingMissingFields) {
                   "parser)(.*)(referenced_errors)(.*)(field:retryThrottling "
                   "field:maxTokens error:Not found)(.*)(field:retryThrottling "
                   "field:tokenRatio error:Not found)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryThrottlingNegativeMaxTokens) {
@@ -621,10 +597,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryThrottlingNegativeMaxTokens) {
                   "Params)(.*)(referenced_errors)(.*)(Client channel global "
                   "parser)(.*)(referenced_errors)(.*)(field:retryThrottling "
                   "field:maxTokens error:should be greater than zero)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryThrottlingInvalidTokenRatio) {
@@ -645,10 +618,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryThrottlingInvalidTokenRatio) {
                   "Params)(.*)(referenced_errors)(.*)(Client channel global "
                   "parser)(.*)(referenced_errors)(.*)(field:retryThrottling "
                   "field:tokenRatio error:Failed parsing)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, ValidTimeout) {
@@ -695,10 +665,7 @@ TEST_F(ClientChannelParserTest, InvalidTimeout) {
                   "referenced_errors)(.*)(Client channel "
                   "parser)(.*)(referenced_errors)(.*)(field:timeout "
                   "error:Failed parsing)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, ValidWaitForReady) {
@@ -751,10 +718,7 @@ TEST_F(ClientChannelParserTest, InvalidWaitForReady) {
                   "referenced_errors)(.*)(Client channel "
                   "parser)(.*)(referenced_errors)(.*)(field:waitForReady "
                   "error:Type should be true/false)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, ValidRetryPolicy) {
@@ -818,11 +782,8 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxAttempts) {
       "Params)(.*)(referenced_errors)(.*)(methodConfig)(.*)(referenced_errors)("
       ".*)(Client channel "
       "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
-      "*)(field:maxAttempts error:should be atleast 2)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+      "*)(field:maxAttempts error:should be at least 2)"));
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryPolicyInitialBackoff) {
@@ -852,10 +813,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyInitialBackoff) {
       ".*)(Client channel "
       "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
       "*)(field:initialBackoff error:Failed to parse)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxBackoff) {
@@ -885,10 +843,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyMaxBackoff) {
       ".*)(Client channel "
       "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
       "*)(field:maxBackoff error:failed to parse)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryPolicyBackoffMultiplier) {
@@ -918,10 +873,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyBackoffMultiplier) {
       ".*)(Client channel "
       "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
       "*)(field:backoffMultiplier error:should be of type number)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) {
@@ -951,10 +903,7 @@ TEST_F(ClientChannelParserTest, InvalidRetryPolicyRetryableStatusCodes) {
       ".*)(Client channel "
       "parser)(.*)(referenced_errors)(.*)(retryPolicy)(.*)(referenced_errors)(."
       "*)(field:retryableStatusCodes error:should be non-empty)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 class MessageSizeParserTest : public ::testing::Test {
@@ -962,7 +911,7 @@ class MessageSizeParserTest : public ::testing::Test {
   void SetUp() override {
     ServiceConfig::Shutdown();
     ServiceConfig::Init();
-    EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr<ServiceConfigParser>(
+    EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser>(
                     New<MessageSizeParser>())) == 0);
   }
 };
@@ -1013,10 +962,7 @@ TEST_F(MessageSizeParserTest, InvalidMaxRequestMessageBytes) {
                   "referenced_errors)(.*)(Message size "
                   "parser)(.*)(referenced_errors)(.*)(field:"
                   "maxRequestMessageBytes error:should be non-negative)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 TEST_F(MessageSizeParserTest, InvalidMaxResponseMessageBytes) {
@@ -1040,10 +986,7 @@ TEST_F(MessageSizeParserTest, InvalidMaxResponseMessageBytes) {
                   "referenced_errors)(.*)(Message size "
                   "parser)(.*)(referenced_errors)(.*)(field:"
                   "maxResponseMessageBytes error:should be of type number)"));
-  std::smatch match;
-  std::string s(grpc_error_string(error));
-  EXPECT_TRUE(std::regex_search(s, match, e));
-  GRPC_ERROR_UNREF(error);
+  VerifyRegexMatch(error, e);
 }
 
 class HealthCheckParserTest : public ::testing::Test {
@@ -1051,7 +994,7 @@ class HealthCheckParserTest : public ::testing::Test {
   void SetUp() override {
     ServiceConfig::Shutdown();
     ServiceConfig::Init();
-    EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr<ServiceConfigParser>(
+    EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr<ServiceConfig::Parser>(
                     New<HealthCheckParser>())) == 0);
   }
 };
@@ -1090,8 +1033,9 @@ TEST_F(HealthCheckParserTest, MultipleEntries) {
   std::regex e(
       std::string("(Service config parsing "
                   "error)(.*)(referenced_errors)(.*)(Global "
-                  "Params)(.*)(referenced_errors)(.*)(field:healthCheckConfig "
-                  "field:serviceName error:Duplicate entry)"));
+                  "Params)(.*)(referenced_errors)(.*)(field:healthCheckConfig)("
+                  ".*)(referenced_errors)(.*)"
+                  "(field:serviceName error:Duplicate entry)"));
   std::smatch match;
   std::string s(grpc_error_string(error));
   EXPECT_TRUE(std::regex_search(s, match, e));

test/cpp/microbenchmarks/bm_call_create.cc

@@ -30,7 +30,6 @@
 #include <grpcpp/support/channel_arguments.h>
 
 #include "src/core/ext/filters/client_channel/client_channel.h"
-#include "src/core/ext/filters/client_channel/client_channel_factory.h"
 #include "src/core/ext/filters/deadline/deadline_filter.h"
 #include "src/core/ext/filters/http/client/http_client_filter.h"
 #include "src/core/ext/filters/http/message_compress/message_compress_filter.h"