From a2316142b237e120f4ac370e608746fb4a440f1a Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 24 Apr 2019 15:29:46 -0700 Subject: [PATCH] Reviewer comments --- .../filters/client_channel/client_channel.cc | 29 ++- .../client_channel/client_channel_factory.h | 1 - .../health/health_check_parser.cc | 32 ++- .../health/health_check_parser.h | 8 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 5 +- .../client_channel/lb_policy/xds/xds.cc | 6 +- .../client_channel/lb_policy_registry.cc | 12 +- .../client_channel/lb_policy_registry.h | 7 +- .../client_channel/resolver_result_parsing.cc | 109 ++++----- .../client_channel/resolver_result_parsing.h | 42 ++-- .../filters/client_channel/service_config.cc | 5 +- .../filters/client_channel/service_config.h | 66 +++--- .../ext/filters/client_channel/subchannel.h | 1 - .../message_size/message_size_filter.cc | 6 +- .../message_size/message_size_filter.h | 6 +- .../client_channel/service_config_test.cc | 208 +++++++----------- test/cpp/microbenchmarks/bm_call_create.cc | 1 - 17 files changed, 260 insertions(+), 284 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 2f13ca7e438..de5a12a4d91 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -762,11 +762,13 @@ class ChannelData::ConnectivityStateAndPickerSetter { class ChannelData::ServiceConfigSetter { public: ServiceConfigSetter( - ChannelData* chand, - RefCountedPtr retry_throttle_data, + ChannelData* chand, UniquePtr server_name, + Optional + retry_throttle_data, RefCountedPtr 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 retry_throttle_data_; + UniquePtr server_name_; + Optional + retry_throttle_data_; RefCountedPtr service_config_; grpc_closure closure_; }; @@ -1120,15 +1130,16 @@ bool ChannelData::ProcessResolverResultLocked( RefCountedPtr* lb_policy_config) { ChannelData* chand = static_cast(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(chand, resolver_result.retry_throttle_data(), - resolver_result.service_config()); + New( + chand, UniquePtr(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(); diff --git a/src/core/ext/filters/client_channel/client_channel_factory.h b/src/core/ext/filters/client_channel/client_channel_factory.h index 883409fbee8..21f78a833df 100644 --- a/src/core/ext/filters/client_channel/client_channel_factory.h +++ b/src/core/ext/filters/client_channel/client_channel_factory.h @@ -23,7 +23,6 @@ #include -#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" diff --git a/src/core/ext/filters/client_channel/health/health_check_parser.cc b/src/core/ext/filters/client_channel/health/health_check_parser.cc index 13bad49c5b6..7760e22b686 100644 --- a/src/core/ext/filters/client_channel/health/health_check_parser.cc +++ b/src/core/ext/filters/client_channel/health/health_check_parser.cc @@ -25,12 +25,14 @@ namespace { size_t g_health_check_parser_index; } -UniquePtr HealthCheckParser::ParseGlobalParams( +UniquePtr HealthCheckParser::ParseGlobalParams( const grpc_json* json, grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); const char* service_name = nullptr; + InlinedVector 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 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( - New(service_name)); + if (error_list.empty()) { + if (service_name == nullptr) return nullptr; + return UniquePtr( + New(service_name)); + } + *error = ServiceConfig::CreateErrorFromVector("field:healthCheckConfig", + &error_list); + return nullptr; } void HealthCheckParser::Register() { g_health_check_parser_index = ServiceConfig::RegisterParser( - UniquePtr(New())); + UniquePtr(New())); } size_t HealthCheckParser::ParserIndex() { return g_health_check_parser_index; } diff --git a/src/core/ext/filters/client_channel/health/health_check_parser.h b/src/core/ext/filters/client_channel/health/health_check_parser.h index 8ab1ba9425d..036cafc68b7 100644 --- a/src/core/ext/filters/client_channel/health/health_check_parser.h +++ b/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 ParseGlobalParams( + UniquePtr ParseGlobalParams( const grpc_json* json, grpc_error** error) override; static void Register(); diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 9580583aff1..c61d6a71527 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/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 pending_child_policy_; // The child policy config. - RefCountedPtr child_policy_config_ = nullptr; + RefCountedPtr child_policy_config_; // Child policy in state READY. bool child_policy_ready_ = false; }; @@ -1812,8 +1812,7 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { New(nullptr)); } InlinedVector error_list; - RefCountedPtr child_policy = nullptr; - + RefCountedPtr child_policy; for (const grpc_json* field = json->child; field != nullptr; field = field->next) { if (field->key == nullptr) continue; diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc index e496a666931..775f45f71b1 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc +++ b/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); diff --git a/src/core/ext/filters/client_channel/lb_policy_registry.cc b/src/core/ext/filters/client_channel/lb_policy_registry.cc index 310d41078f7..b9ea97d4051 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.cc +++ b/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"); diff --git a/src/core/ext/filters/client_channel/lb_policy_registry.h b/src/core/ext/filters/client_channel/lb_policy_registry.h index 298d73a6312..7fa52914cd9 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.h +++ b/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 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 diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.cc b/src/core/ext/filters/client_channel/resolver_result_parsing.cc index 36192a5929e..e868cd70bc6 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -29,6 +29,7 @@ #include #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(New())); + g_client_channel_service_config_parser_index = + ServiceConfig::RegisterParser(UniquePtr( + New())); } 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( + 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( 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( - 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( - 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 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 ParseRetryPolicy( } // namespace -UniquePtr +UniquePtr ClientChannelServiceConfigParser::ParseGlobalParams(const grpc_json* json, grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); InlinedVector error_list; RefCountedPtr parsed_lb_config; - const char* lb_policy_name = nullptr; + UniquePtr lb_policy_name; Optional 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( + return UniquePtr( New(std::move(parsed_lb_config), - lb_policy_name, retry_throttling)); + std::move(lb_policy_name), + retry_throttling)); } return nullptr; } -UniquePtr +UniquePtr 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( + return UniquePtr( New(timeout, wait_for_ready, std::move(retry_policy))); } diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.h b/src/core/ext/filters/client_channel/resolver_result_parsing.h index 097f1e39e62..25830dd4642 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.h +++ b/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 parsed_lb_config, - const char* parsed_deprecated_lb_policy, + UniquePtr parsed_deprecated_lb_policy, const Optional& 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 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 parsed_lb_config_; - const char* parsed_deprecated_lb_policy_ = nullptr; + UniquePtr parsed_deprecated_lb_policy_; Optional 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 retry_policy_; }; -class ClientChannelServiceConfigParser : public ServiceConfigParser { +class ClientChannelServiceConfigParser : public ServiceConfig::Parser { public: - UniquePtr ParseGlobalParams( + UniquePtr ParseGlobalParams( const grpc_json* json, grpc_error** error) override; - UniquePtr ParsePerMethodParams( + UniquePtr 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 retry_throttle_data() { - return std::move(retry_throttle_data_); + + const char* server_name() { return server_name_; } + + Optional + retry_throttle_data() { + return retry_throttle_data_; } UniquePtr 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 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 lb_policy_name_; RefCountedPtr lb_policy_config_ = nullptr; // Retry throttle data. - RefCountedPtr retry_throttle_data_; + const char* server_name_ = nullptr; + Optional + retry_throttle_data_; const char* health_check_service_name_ = nullptr; }; diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc index 9c2fb254db8..8db3df712e6 100644 --- a/src/core/ext/filters/client_channel/service_config.cc +++ b/src/core/ext/filters/client_channel/service_config.cc @@ -34,7 +34,7 @@ namespace grpc_core { namespace { -typedef InlinedVector, +typedef InlinedVector, ServiceConfig::kNumPreallocatedParsers> ServiceConfigParserList; ServiceConfigParserList* g_registered_parsers; @@ -78,7 +78,6 @@ ServiceConfig::ServiceConfig(UniquePtr 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 parser) { +size_t ServiceConfig::RegisterParser(UniquePtr parser) { g_registered_parsers->push_back(std::move(parser)); return g_registered_parsers->size() - 1; } diff --git a/src/core/ext/filters/client_channel/service_config.h b/src/core/ext/filters/client_channel/service_config.h index 85a49855f96..1372b50eb2a 100644 --- a/src/core/ext/filters/client_channel/service_config.h +++ b/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 { 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 ParseGlobalParams( - const grpc_json* json, grpc_error** error) { - GPR_DEBUG_ASSERT(error != nullptr); - return nullptr; - } + virtual UniquePtr ParseGlobalParams( + const grpc_json* json, grpc_error** error) { + GPR_DEBUG_ASSERT(error != nullptr); + return nullptr; + } - virtual UniquePtr ParsePerMethodParams( - const grpc_json* json, grpc_error** error) { - GPR_DEBUG_ASSERT(error != nullptr); - return nullptr; - } + virtual UniquePtr 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 { - public: static constexpr int kNumPreallocatedParsers = 4; - typedef InlinedVector, + typedef InlinedVector, kNumPreallocatedParsers> ServiceConfigObjectsVector; @@ -104,7 +104,7 @@ class ServiceConfig : public RefCounted { RefCountedPtr 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 { 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 { /// 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 parser); + static size_t RegisterParser(UniquePtr parser); static void Init(); @@ -199,7 +203,7 @@ class ServiceConfig : public RefCounted { UniquePtr json_string_; // Underlying storage for json_tree. grpc_json* json_tree_; - InlinedVector, kNumPreallocatedParsers> + InlinedVector, 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 diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index c67b51a8fa4..9c2e57d3e05 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/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" diff --git a/src/core/ext/filters/message_size/message_size_filter.cc b/src/core/ext/filters/message_size/message_size_filter.cc index e19accbe8a7..2014855e5d1 100644 --- a/src/core/ext/filters/message_size/message_size_filter.cc +++ b/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 MessageSizeParser::ParsePerMethodParams( +UniquePtr 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 MessageSizeParser::ParsePerMethodParams( &error_list); return nullptr; } - return UniquePtr(New( + return UniquePtr(New( max_request_message_bytes, max_response_message_bytes)); } void MessageSizeParser::Register() { g_message_size_parser_index = ServiceConfig::RegisterParser( - UniquePtr(New())); + UniquePtr(New())); } size_t MessageSizeParser::ParserIndex() { return g_message_size_parser_index; } diff --git a/src/core/ext/filters/message_size/message_size_filter.h b/src/core/ext/filters/message_size/message_size_filter.h index e43d7be61b5..cab8bd9627f 100644 --- a/src/core/ext/filters/message_size/message_size_filter.h +++ b/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 ParsePerMethodParams( + UniquePtr ParsePerMethodParams( const grpc_json* json, grpc_error** error) override; static void Register(); diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index 544d91ec02e..fd5304c17d4 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/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 ParseGlobalParams( + UniquePtr 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( + return UniquePtr( New(value)); } } @@ -77,9 +77,9 @@ class TestParser1 : public ServiceConfigParser { } }; -class TestParser2 : public ServiceConfigParser { +class TestParser2 : public ServiceConfig::Parser { public: - UniquePtr ParsePerMethodParams( + UniquePtr 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( + return UniquePtr( New(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 ParsePerMethodParams( + UniquePtr 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 ParseGlobalParams( + UniquePtr 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(New())) == 0); + UniquePtr(New())) == 0); EXPECT_TRUE(ServiceConfig::RegisterParser( - UniquePtr(New())) == 1); + UniquePtr(New())) == 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(New())) == 0); + UniquePtr(New())) == 0); EXPECT_TRUE(ServiceConfig::RegisterParser( - UniquePtr(New())) == 1); + UniquePtr(New())) == 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( + ServiceConfig::RegisterParser(UniquePtr( New())) == 0); } @@ -395,7 +375,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigPickFirst) { const auto* parsed_object = static_cast( 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( 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( 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( 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( + 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( + EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr( New())) == 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( + EXPECT_TRUE(ServiceConfig::RegisterParser(UniquePtr( New())) == 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)); diff --git a/test/cpp/microbenchmarks/bm_call_create.cc b/test/cpp/microbenchmarks/bm_call_create.cc index 177b0187cc6..e84999b213f 100644 --- a/test/cpp/microbenchmarks/bm_call_create.cc +++ b/test/cpp/microbenchmarks/bm_call_create.cc @@ -30,7 +30,6 @@ #include #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"