diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 30c96331012..12aaf43fb0b 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1135,8 +1135,9 @@ RefCountedPtr ChooseLbPolicy( Json config_json = Json::Array{Json::Object{ {std::string(*policy_name), Json::Object{}}, }}; - auto lb_policy_config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(config_json); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + auto lb_policy_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + config_json, &parse_error); // The policy name came from one of three places: // - The deprecated loadBalancingPolicy field in the service config, // in which case the code in ClientChannelServiceConfigParser @@ -1146,8 +1147,9 @@ RefCountedPtr ChooseLbPolicy( // - A channel arg, in which case we check that the specified policy exists // and accepts an empty config. If not, we revert to using pick_first // lb_policy - GPR_ASSERT(lb_policy_config.ok()); - return std::move(*lb_policy_config); + GPR_ASSERT(lb_policy_config != nullptr); + GPR_ASSERT(GRPC_ERROR_IS_NONE(parse_error)); + return lb_policy_config; } } // namespace @@ -1246,9 +1248,9 @@ void ClientChannel::OnResolverResultChangedLocked(Resolver::Result result) { // If either has changed, apply the global parameters now. if (service_config_changed || config_selector_changed) { // Update service config in control plane. - UpdateServiceConfigInControlPlaneLocked( - std::move(service_config), std::move(config_selector), - std::string(lb_policy_config->name())); + UpdateServiceConfigInControlPlaneLocked(std::move(service_config), + std::move(config_selector), + lb_policy_config->name()); } else if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_trace)) { gpr_log(GPR_INFO, "chand=%p: service config not changed", this); } diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index 1838929498a..12629e89e41 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -305,7 +305,7 @@ class LoadBalancingPolicy : public InternallyRefCounted { ~Config() override = default; // Returns the load balancing policy name - virtual absl::string_view name() const = 0; + virtual const char* name() const = 0; }; /// Data passed to the UpdateLocked() method when new addresses and @@ -349,7 +349,7 @@ class LoadBalancingPolicy : public InternallyRefCounted { LoadBalancingPolicy& operator=(const LoadBalancingPolicy&) = delete; /// Returns the name of the LB policy. - virtual absl::string_view name() const = 0; + virtual const char* name() const = 0; /// Updates the policy with new data from the resolver. Will be invoked /// immediately after LB policy is constructed, and then again whenever diff --git a/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc b/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc index d6d687ee591..bc3a4ed37a5 100644 --- a/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc +++ b/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.cc @@ -18,6 +18,7 @@ #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" +#include #include #include @@ -229,8 +230,7 @@ void ChildPolicyHandler::UpdateLocked(UpdateArgs args) { if (GRPC_TRACE_FLAG_ENABLED(*tracer_)) { gpr_log(GPR_INFO, "[child_policy_handler %p] creating new %schild policy %s", this, - child_policy_ == nullptr ? "" : "pending ", - std::string(args.config->name()).c_str()); + child_policy_ == nullptr ? "" : "pending ", args.config->name()); } auto& lb_policy = child_policy_ == nullptr ? child_policy_ : pending_child_policy_; @@ -274,7 +274,7 @@ void ChildPolicyHandler::ResetBackoffLocked() { } OrphanablePtr ChildPolicyHandler::CreateChildPolicy( - absl::string_view child_policy_name, const ChannelArgs& args) { + const char* child_policy_name, const ChannelArgs& args) { Helper* helper = new Helper(Ref(DEBUG_LOCATION, "Helper")); LoadBalancingPolicy::Args lb_policy_args; lb_policy_args.work_serializer = work_serializer(); @@ -284,15 +284,14 @@ OrphanablePtr ChildPolicyHandler::CreateChildPolicy( OrphanablePtr lb_policy = CreateLoadBalancingPolicy(child_policy_name, std::move(lb_policy_args)); if (GPR_UNLIKELY(lb_policy == nullptr)) { - gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", - std::string(child_policy_name).c_str()); + gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", child_policy_name); return nullptr; } helper->set_child(lb_policy.get()); if (GRPC_TRACE_FLAG_ENABLED(*tracer_)) { gpr_log(GPR_INFO, "[child_policy_handler %p] created new LB policy \"%s\" (%p)", this, - std::string(child_policy_name).c_str(), lb_policy.get()); + child_policy_name, lb_policy.get()); } channel_control_helper()->AddTraceEvent( ChannelControlHelper::TRACE_INFO, @@ -305,12 +304,12 @@ OrphanablePtr ChildPolicyHandler::CreateChildPolicy( bool ChildPolicyHandler::ConfigChangeRequiresNewPolicyInstance( LoadBalancingPolicy::Config* old_config, LoadBalancingPolicy::Config* new_config) const { - return old_config->name() != new_config->name(); + return strcmp(old_config->name(), new_config->name()) != 0; } OrphanablePtr ChildPolicyHandler::CreateLoadBalancingPolicy( - absl::string_view name, LoadBalancingPolicy::Args args) const { + const char* name, LoadBalancingPolicy::Args args) const { return LoadBalancingPolicyRegistry::CreateLoadBalancingPolicy( name, std::move(args)); } diff --git a/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h b/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h index 9b7c4096f94..d3093518e02 100644 --- a/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h +++ b/src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h @@ -16,12 +16,11 @@ #ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_CHILD_POLICY_HANDLER_H #define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_CHILD_POLICY_HANDLER_H + #include #include -#include "absl/strings/string_view.h" - #include "src/core/ext/filters/client_channel/lb_policy.h" #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/debug/trace.h" @@ -41,7 +40,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { ChildPolicyHandler(Args args, TraceFlag* tracer) : LoadBalancingPolicy(std::move(args)), tracer_(tracer) {} - absl::string_view name() const override { return "child_policy_handler"; } + const char* name() const override { return "child_policy_handler"; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -57,7 +56,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { // May be overridden by subclasses to avoid recursion when an LB // policy factory returns a ChildPolicyHandler. virtual OrphanablePtr CreateLoadBalancingPolicy( - absl::string_view name, LoadBalancingPolicy::Args args) const; + const char* name, LoadBalancingPolicy::Args args) const; private: class Helper; @@ -65,7 +64,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { void ShutdownLocked() override; OrphanablePtr CreateChildPolicy( - absl::string_view child_policy_name, const ChannelArgs& args); + const char* child_policy_name, const ChannelArgs& args); // Passed in from caller at construction time. TraceFlag* tracer_; 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 c7e01a7b29a..3570fe46450 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 @@ -160,7 +160,7 @@ namespace { using ::grpc_event_engine::experimental::EventEngine; using ::grpc_event_engine::experimental::GetDefaultEventEngine; -constexpr absl::string_view kGrpclb = "grpclb"; +constexpr char kGrpclb[] = "grpclb"; class GrpcLbConfig : public LoadBalancingPolicy::Config { public: @@ -168,8 +168,7 @@ class GrpcLbConfig : public LoadBalancingPolicy::Config { std::string service_name) : child_policy_(std::move(child_policy)), service_name_(std::move(service_name)) {} - - absl::string_view name() const override { return kGrpclb; } + const char* name() const override { return kGrpclb; } RefCountedPtr child_policy() const { return child_policy_; @@ -186,7 +185,7 @@ class GrpcLb : public LoadBalancingPolicy { public: explicit GrpcLb(Args args); - absl::string_view name() const override { return kGrpclb; } + const char* name() const override { return kGrpclb; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -1846,27 +1845,28 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kGrpclb; } + const char* name() const override { return kGrpclb; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { return MakeRefCounted(nullptr, ""); } - std::vector error_list; + std::vector error_list; + Json child_policy_config_json_tmp; + const Json* child_policy_config_json; std::string service_name; auto it = json.object_value().find("serviceName"); if (it != json.object_value().end()) { const Json& service_name_json = it->second; if (service_name_json.type() != Json::Type::STRING) { - error_list.emplace_back( - "field:serviceName error:type should be string"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:serviceName error:type should be string")); } else { service_name = service_name_json.string_value(); } } - Json child_policy_config_json_tmp; - const Json* child_policy_config_json; it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { child_policy_config_json_tmp = Json::Array{Json::Object{ @@ -1876,24 +1876,25 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { } else { child_policy_config_json = &it->second; } - auto child_policy_config = + grpc_error_handle parse_error = GRPC_ERROR_NONE; + RefCountedPtr child_policy_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - *child_policy_config_json); - if (!child_policy_config.ok()) { - error_list.emplace_back( - absl::StrCat("error parsing childPolicy field: ", - child_policy_config.status().message())); + *child_policy_config_json, &parse_error); + if (!GRPC_ERROR_IS_NONE(parse_error)) { + std::vector child_errors; + child_errors.push_back(parse_error); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_VECTOR("field:childPolicy", &child_errors)); } if (error_list.empty()) { - return MakeRefCounted(std::move(*child_policy_config), + return MakeRefCounted(std::move(child_policy_config), std::move(service_name)); } else { - return absl::InvalidArgumentError( - absl::StrCat("errors parsing grpclb LB policy config: [", - absl::StrJoin(error_list, "; "), "]")); + *error = GRPC_ERROR_CREATE_FROM_VECTOR("GrpcLb Parser", &error_list); + return nullptr; } } -}; +}; // namespace grpc_core } // namespace diff --git a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc index 4d91935a030..2b6ab6096a2 100644 --- a/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc +++ b/src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc @@ -36,8 +36,6 @@ #include "absl/random/random.h" #include "absl/status/status.h" #include "absl/status/statusor.h" -#include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/variant.h" @@ -86,8 +84,7 @@ bool XdsOutlierDetectionEnabled() { namespace { -constexpr absl::string_view kOutlierDetection = - "outlier_detection_experimental"; +constexpr char kOutlierDetection[] = "outlier_detection_experimental"; // Config for xDS Cluster Impl LB policy. class OutlierDetectionLbConfig : public LoadBalancingPolicy::Config { @@ -98,7 +95,7 @@ class OutlierDetectionLbConfig : public LoadBalancingPolicy::Config { : outlier_detection_config_(outlier_detection_config), child_policy_(std::move(child_policy)) {} - absl::string_view name() const override { return kOutlierDetection; } + const char* name() const override { return kOutlierDetection; } bool CountingEnabled() const { return ( @@ -125,7 +122,7 @@ class OutlierDetectionLb : public LoadBalancingPolicy { public: explicit OutlierDetectionLb(Args args); - absl::string_view name() const override { return kOutlierDetection; } + const char* name() const override { return kOutlierDetection; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -1011,27 +1008,28 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kOutlierDetection; } + const char* name() const override { return kOutlierDetection; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // This policy was configured in the deprecated loadBalancingPolicy // field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:outlier_detection policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); + return nullptr; } - std::vector errors; std::vector error_list; // Outlier detection config OutlierDetectionConfig outlier_detection_config; auto it = json.object_value().find("successRateEjection"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back( - "field:successRateEjection error:type must be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:successRateEjection error:type must be object")); } else { OutlierDetectionConfig::SuccessRateEjection success_config; const Json::Object& object = it->second.object_value(); @@ -1053,8 +1051,8 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("failurePercentageEjection"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back( - "field:successRateEjection error:type must be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:successRateEjection error:type must be object")); } else { OutlierDetectionConfig::FailurePercentageEjection failure_config; const Json::Object& object = it->second.object_value(); @@ -1091,26 +1089,24 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { RefCountedPtr child_policy; it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - errors.emplace_back("field:childPolicy error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:childPolicy error:required field missing")); } else { - auto child_policy_config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second); - if (!child_policy_config.ok()) { - errors.emplace_back( - absl::StrCat("error parsing childPolicy field: ", - child_policy_config.status().message())); - } else { - child_policy = std::move(*child_policy_config); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + child_policy = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + it->second, &parse_error); + if (child_policy == nullptr) { + GPR_DEBUG_ASSERT(!GRPC_ERROR_IS_NONE(parse_error)); + std::vector child_errors; + child_errors.push_back(parse_error); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_VECTOR("field:childPolicy", &child_errors)); } } - for (auto& error : error_list) { - errors.emplace_back(grpc_error_std_string(error)); - GRPC_ERROR_UNREF(error); - } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("outlier_detection_experimental LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (!error_list.empty()) { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "outlier_detection_experimental LB policy config", &error_list); + return nullptr; } return MakeRefCounted(outlier_detection_config, std::move(child_policy)); diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index 923d8ee0f49..3435948a434 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -29,7 +29,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include @@ -46,6 +45,7 @@ #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" #include "src/core/lib/resolver/server_address.h" #include "src/core/lib/transport/connectivity_state.h" @@ -60,13 +60,13 @@ namespace { // pick_first LB policy // -constexpr absl::string_view kPickFirst = "pick_first"; +constexpr char kPickFirst[] = "pick_first"; class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(Args args); - absl::string_view name() const override { return kPickFirst; } + const char* name() const override { return kPickFirst; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -502,7 +502,7 @@ void PickFirst::PickFirstSubchannelData::ProcessUnselectedReadyLocked() { class PickFirstConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kPickFirst; } + const char* name() const override { return kPickFirst; } }; // @@ -516,10 +516,10 @@ class PickFirstFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kPickFirst; } + const char* name() const override { return kPickFirst; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc index 4b5e7f92959..97640431145 100644 --- a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc +++ b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc @@ -30,7 +30,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -67,7 +66,7 @@ TraceFlag grpc_lb_priority_trace(false, "priority_lb"); namespace { -constexpr absl::string_view kPriority = "priority_experimental"; +constexpr char kPriority[] = "priority_experimental"; // How long we keep a child around for after it is no longer being used // (either because it has been removed from the config or because we @@ -90,7 +89,7 @@ class PriorityLbConfig : public LoadBalancingPolicy::Config { std::vector priorities) : children_(std::move(children)), priorities_(std::move(priorities)) {} - absl::string_view name() const override { return kPriority; } + const char* name() const override { return kPriority; } const std::map& children() const { return children_; @@ -107,7 +106,7 @@ class PriorityLb : public LoadBalancingPolicy { public: explicit PriorityLb(Args args); - absl::string_view name() const override { return kPriority; } + const char* name() const override { return kPriority; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -927,40 +926,49 @@ class PriorityLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kPriority; } + const char* name() const override { return kPriority; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // priority was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:priority policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); + return nullptr; } - std::vector errors; + std::vector error_list; // Children. std::map children; auto it = json.object_value().find("children"); if (it == json.object_value().end()) { - errors.emplace_back("field:children error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:children error:required field missing")); } else if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back("field:children error:type should be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:children error:type should be object")); } else { const Json::Object& object = it->second.object_value(); for (const auto& p : object) { const std::string& child_name = p.first; const Json& element = p.second; if (element.type() != Json::Type::OBJECT) { - errors.emplace_back(absl::StrCat("field:children key:", child_name, - " error:should be type object")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( + absl::StrCat("field:children key:", child_name, + " error:should be type object"))); } else { auto it2 = element.object_value().find("config"); if (it2 == element.object_value().end()) { - errors.emplace_back(absl::StrCat("field:children key:", child_name, - " error:missing 'config' field")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( + absl::StrCat("field:children key:", child_name, + " error:missing 'config' field"))); } else { + grpc_error_handle parse_error = GRPC_ERROR_NONE; + auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + it2->second, &parse_error); bool ignore_resolution_requests = false; // If present, ignore_reresolution_requests must be of type // boolean. @@ -970,23 +978,23 @@ class PriorityLbFactory : public LoadBalancingPolicyFactory { if (it3->second.type() == Json::Type::JSON_TRUE) { ignore_resolution_requests = true; } else if (it3->second.type() != Json::Type::JSON_FALSE) { - errors.emplace_back( + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( absl::StrCat("field:children key:", child_name, " field:ignore_reresolution_requests:should " - "be type boolean")); + "be type boolean"))); } } - auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - it2->second); - if (!config.ok()) { - errors.emplace_back( - absl::StrCat("field:children key:", child_name, ": ", - config.status().message())); - } else { - children[child_name].config = std::move(*config); - children[child_name].ignore_reresolution_requests = - ignore_resolution_requests; + if (config == nullptr) { + GPR_DEBUG_ASSERT(!GRPC_ERROR_IS_NONE(parse_error)); + error_list.push_back( + GRPC_ERROR_CREATE_REFERENCING_FROM_COPIED_STRING( + absl::StrCat("field:children key:", child_name).c_str(), + &parse_error, 1)); + GRPC_ERROR_UNREF(parse_error); } + children[child_name].config = std::move(config); + children[child_name].ignore_reresolution_requests = + ignore_resolution_requests; } } } @@ -995,37 +1003,40 @@ class PriorityLbFactory : public LoadBalancingPolicyFactory { std::vector priorities; it = json.object_value().find("priorities"); if (it == json.object_value().end()) { - errors.emplace_back("field:priorities error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:priorities error:required field missing")); } else if (it->second.type() != Json::Type::ARRAY) { - errors.emplace_back("field:priorities error:type should be array"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:priorities error:type should be array")); } else { const Json::Array& array = it->second.array_value(); for (size_t i = 0; i < array.size(); ++i) { const Json& element = array[i]; if (element.type() != Json::Type::STRING) { - errors.emplace_back(absl::StrCat("field:priorities element:", i, - " error:should be type string")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( + "field:priorities element:", i, " error:should be type string"))); } else if (children.find(element.string_value()) == children.end()) { - errors.emplace_back(absl::StrCat("field:priorities element:", i, - " error:unknown child '", - element.string_value(), "'")); + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( + "field:priorities element:", i, " error:unknown child '", + element.string_value(), "'"))); } else { priorities.emplace_back(element.string_value()); } } if (priorities.size() != children.size()) { - errors.emplace_back(absl::StrCat( + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( "field:priorities error:priorities size (", priorities.size(), - ") != children size (", children.size(), ")")); + ") != children size (", children.size(), ")"))); } } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("priority_experimental LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (error_list.empty()) { + return MakeRefCounted(std::move(children), + std::move(priorities)); + } else { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "priority_experimental LB policy config", &error_list); + return nullptr; } - return MakeRefCounted(std::move(children), - std::move(priorities)); } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc index 7976956db98..56d5ef12337 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc @@ -16,8 +16,6 @@ #include -#include "src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h" - #include #include @@ -38,10 +36,11 @@ #include "absl/status/statusor.h" #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "src/core/lib/channel/channel_args.h" + #define XXH_INLINE_ALL #include "xxhash.h" @@ -55,7 +54,6 @@ #include "src/core/ext/filters/client_channel/lb_policy_registry.h" #include "src/core/ext/filters/client_channel/subchannel_interface.h" #include "src/core/lib/address_utils/sockaddr_utils.h" -#include "src/core/lib/channel/channel_args.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/debug_location.h" @@ -82,60 +80,56 @@ UniqueTypeName RequestHashAttributeName() { } // Helper Parser method -absl::StatusOr ParseRingHashLbConfig(const Json& json) { +void ParseRingHashLbConfig(const Json& json, size_t* min_ring_size, + size_t* max_ring_size, + std::vector* error_list) { + *min_ring_size = 1024; + *max_ring_size = 8388608; if (json.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError( - "ring_hash_experimental should be of type object"); + error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "ring_hash_experimental should be of type object")); + return; } - RingHashConfig config; - std::vector errors; const Json::Object& ring_hash = json.object_value(); auto ring_hash_it = ring_hash.find("min_ring_size"); if (ring_hash_it != ring_hash.end()) { if (ring_hash_it->second.type() != Json::Type::NUMBER) { - errors.emplace_back( - "field:min_ring_size error: should be of type number"); + error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:min_ring_size error: should be of type number")); } else { - config.min_ring_size = gpr_parse_nonnegative_int( + *min_ring_size = gpr_parse_nonnegative_int( ring_hash_it->second.string_value().c_str()); } } ring_hash_it = ring_hash.find("max_ring_size"); if (ring_hash_it != ring_hash.end()) { if (ring_hash_it->second.type() != Json::Type::NUMBER) { - errors.emplace_back( - "field:max_ring_size error: should be of type number"); + error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:max_ring_size error: should be of type number")); } else { - config.max_ring_size = gpr_parse_nonnegative_int( + *max_ring_size = gpr_parse_nonnegative_int( ring_hash_it->second.string_value().c_str()); } } - if (config.min_ring_size == 0 || config.min_ring_size > 8388608 || - config.max_ring_size == 0 || config.max_ring_size > 8388608 || - config.min_ring_size > config.max_ring_size) { - errors.emplace_back( + if (*min_ring_size == 0 || *min_ring_size > 8388608 || *max_ring_size == 0 || + *max_ring_size > 8388608 || *min_ring_size > *max_ring_size) { + error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:max_ring_size and or min_ring_size error: " "values need to be in the range of 1 to 8388608 " "and max_ring_size cannot be smaller than " - "min_ring_size"); + "min_ring_size")); } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("errors parsing ring hash LB config: [", - absl::StrJoin(errors, "; "), "]")); - } - return config; } namespace { -constexpr absl::string_view kRingHash = "ring_hash_experimental"; +constexpr char kRingHash[] = "ring_hash_experimental"; class RingHashLbConfig : public LoadBalancingPolicy::Config { public: RingHashLbConfig(size_t min_ring_size, size_t max_ring_size) : min_ring_size_(min_ring_size), max_ring_size_(max_ring_size) {} - absl::string_view name() const override { return kRingHash; } + const char* name() const override { return kRingHash; } size_t min_ring_size() const { return min_ring_size_; } size_t max_ring_size() const { return max_ring_size_; } @@ -152,7 +146,7 @@ class RingHash : public LoadBalancingPolicy { public: explicit RingHash(Args args); - absl::string_view name() const override { return kRingHash; } + const char* name() const override { return kRingHash; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -880,14 +874,21 @@ class RingHashFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kRingHash; } + const char* name() const override { return kRingHash; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { - auto config = ParseRingHashLbConfig(json); - if (!config.ok()) return config.status(); - return MakeRefCounted(config->min_ring_size, - config->max_ring_size); + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + size_t min_ring_size; + size_t max_ring_size; + std::vector error_list; + ParseRingHashLbConfig(json, &min_ring_size, &max_ring_size, &error_list); + if (error_list.empty()) { + return MakeRefCounted(min_ring_size, max_ring_size); + } else { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "ring_hash_experimental LB policy config", &error_list); + return nullptr; + } } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h index 89ddab0b831..fdc0b10278b 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h @@ -21,9 +21,10 @@ #include -#include "absl/status/statusor.h" +#include #include "src/core/lib/gprpp/unique_type_name.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" namespace grpc_core { @@ -32,11 +33,9 @@ UniqueTypeName RequestHashAttributeName(); // Helper Parsing method to parse ring hash policy configs; for example, ring // hash size validity. -struct RingHashConfig { - size_t min_ring_size = 1024; - size_t max_ring_size = 8388608; -}; -absl::StatusOr ParseRingHashLbConfig(const Json& json); +void ParseRingHashLbConfig(const Json& json, size_t* min_ring_size, + size_t* max_ring_size, + std::vector* error_list); } // namespace grpc_core diff --git a/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc b/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc index 9bd016b6200..94a57cfe215 100644 --- a/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc +++ b/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc @@ -107,7 +107,7 @@ TraceFlag grpc_lb_rls_trace(false, "rls_lb"); namespace { -constexpr absl::string_view kRls = "rls_experimental"; +const char* kRls = "rls_experimental"; const char kGrpc[] = "grpc"; const char* kRlsRequestPath = "/grpc.lookup.v1.RouteLookupService/RouteLookup"; const char* kFakeTargetFieldValue = "fake_target_field_value"; @@ -162,7 +162,7 @@ class RlsLbConfig : public LoadBalancingPolicy::Config { default_child_policy_parsed_config_( std::move(default_child_policy_parsed_config)) {} - absl::string_view name() const override { return kRls; } + const char* name() const override { return kRls; } const KeyBuilderMap& key_builder_map() const { return route_lookup_config_.key_builder_map; @@ -207,7 +207,7 @@ class RlsLb : public LoadBalancingPolicy { public: explicit RlsLb(Args args); - absl::string_view name() const override { return kRls; } + const char* name() const override { return kRls; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; void ResetBackoffLocked() override; @@ -790,23 +790,23 @@ void RlsLb::ChildPolicyWrapper::StartUpdate() { lb_policy_.get(), this, target_.c_str(), child_policy_config.Dump().c_str()); } - auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - child_policy_config); + pending_config_ = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + child_policy_config, &error); // Returned RLS target fails the validation. - if (!config.ok()) { + if (!GRPC_ERROR_IS_NONE(error)) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_rls_trace)) { gpr_log(GPR_INFO, "[rlslb %p] ChildPolicyWrapper=%p [%s]: config failed to parse: " - "%s", + "%s; config: %s", lb_policy_.get(), this, target_.c_str(), - config.status().ToString().c_str()); + grpc_error_std_string(error).c_str(), + child_policy_config.Dump().c_str()); } pending_config_.reset(); picker_ = absl::make_unique( - absl::UnavailableError(config.status().message())); + grpc_error_to_absl_status(error)); + GRPC_ERROR_UNREF(error); child_policy_.reset(); - } else { - pending_config_ = std::move(*config); } } @@ -2441,42 +2441,43 @@ grpc_error_handle ValidateChildPolicyList( child_policy_config_target_field_name, target, child_policy_config); if (!GRPC_ERROR_IS_NONE(error)) return error; // Parse the config. - auto parsed_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - *child_policy_config); - if (!parsed_config.ok()) { - return absl_status_to_grpc_error(parsed_config.status()); - } + RefCountedPtr parsed_config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + *child_policy_config, &error); + if (!GRPC_ERROR_IS_NONE(error)) return error; // Find the chosen config and return it in JSON form. // We remove all non-selected configs, and in the selected config, we leave // the target field in place, set to the default value. This slightly // optimizes what we need to do later when we update a child policy for a // given target. - for (Json& config : *(child_policy_config->mutable_array())) { - if (config.object_value().begin()->first == (*parsed_config)->name()) { - Json save_config = std::move(config); - child_policy_config->mutable_array()->clear(); - child_policy_config->mutable_array()->push_back(std::move(save_config)); - break; + if (parsed_config != nullptr) { + for (Json& config : *(child_policy_config->mutable_array())) { + if (config.object_value().begin()->first == parsed_config->name()) { + Json save_config = std::move(config); + child_policy_config->mutable_array()->clear(); + child_policy_config->mutable_array()->push_back(std::move(save_config)); + break; + } } } // If default target is set, return the parsed config. if (!default_target.empty()) { - *default_child_policy_parsed_config = std::move(*parsed_config); + *default_child_policy_parsed_config = std::move(parsed_config); } return GRPC_ERROR_NONE; } class RlsLbFactory : public LoadBalancingPolicyFactory { public: - absl::string_view name() const override { return kRls; } + const char* name() const override { return kRls; } OrphanablePtr CreateLoadBalancingPolicy( LoadBalancingPolicy::Args args) const override { return MakeOrphanable(std::move(args)); } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& config) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& config, grpc_error_handle* error) const override { std::vector error_list; // Parse routeLookupConfig. RlsLbConfig::RouteLookupConfig route_lookup_config; @@ -2541,13 +2542,8 @@ class RlsLbFactory : public LoadBalancingPolicyFactory { } } // Return result. - if (!error_list.empty()) { - grpc_error_handle error = GRPC_ERROR_CREATE_FROM_VECTOR( - "errors parsing RLS LB policy config", &error_list); - std::string error_string = grpc_error_std_string(error); - GRPC_ERROR_UNREF(error); - return absl::InvalidArgumentError(error_string); - } + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "errors parsing RLS LB policy config", &error_list); return MakeRefCounted( std::move(route_lookup_config), std::move(rls_channel_service_config), std::move(child_policy_config), diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index da960fb6b95..c10f8975463 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -29,7 +29,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include @@ -45,6 +44,7 @@ #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" #include "src/core/lib/resolver/server_address.h" #include "src/core/lib/transport/connectivity_state.h" @@ -59,13 +59,13 @@ namespace { // round_robin LB policy // -constexpr absl::string_view kRoundRobin = "round_robin"; +constexpr char kRoundRobin[] = "round_robin"; class RoundRobin : public LoadBalancingPolicy { public: explicit RoundRobin(Args args); - absl::string_view name() const override { return kRoundRobin; } + const char* name() const override { return kRoundRobin; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -499,7 +499,7 @@ void RoundRobin::RoundRobinSubchannelData::UpdateLogicalConnectivityStateLocked( class RoundRobinConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kRoundRobin; } + const char* name() const override { return kRoundRobin; } }; class RoundRobinFactory : public LoadBalancingPolicyFactory { @@ -509,10 +509,10 @@ class RoundRobinFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kRoundRobin; } + const char* name() const override { return kRoundRobin; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc index 2cf75212224..cfc0feb840e 100644 --- a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc +++ b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc @@ -30,7 +30,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -54,6 +53,7 @@ #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/gprpp/work_serializer.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/pollset_set.h" #include "src/core/lib/json/json.h" #include "src/core/lib/resolver/server_address.h" @@ -70,7 +70,7 @@ namespace { using ::grpc_event_engine::experimental::EventEngine; using ::grpc_event_engine::experimental::GetDefaultEventEngine; -constexpr absl::string_view kWeightedTarget = "weighted_target_experimental"; +constexpr char kWeightedTarget[] = "weighted_target_experimental"; // How long we keep a child around for after it has been removed from // the config. @@ -89,7 +89,7 @@ class WeightedTargetLbConfig : public LoadBalancingPolicy::Config { explicit WeightedTargetLbConfig(TargetMap target_map) : target_map_(std::move(target_map)) {} - absl::string_view name() const override { return kWeightedTarget; } + const char* name() const override { return kWeightedTarget; } const TargetMap& target_map() const { return target_map_; } @@ -102,7 +102,7 @@ class WeightedTargetLb : public LoadBalancingPolicy { public: explicit WeightedTargetLb(Args args); - absl::string_view name() const override { return kWeightedTarget; } + const char* name() const override { return kWeightedTarget; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -683,87 +683,96 @@ class WeightedTargetLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kWeightedTarget; } + const char* name() const override { return kWeightedTarget; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // weighted_target was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:weighted_target policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); + return nullptr; } - std::vector errors; + std::vector error_list; // Weight map. WeightedTargetLbConfig::TargetMap target_map; auto it = json.object_value().find("targets"); if (it == json.object_value().end()) { - errors.emplace_back("field:targets error:required field not present"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:targets error:required field not present")); } else if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back("field:targets error:type should be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:targets error:type should be object")); } else { for (const auto& p : it->second.object_value()) { - auto config = ParseChildConfig(p.second); - if (!config.ok()) { - errors.emplace_back(config.status().message()); + WeightedTargetLbConfig::ChildConfig child_config; + std::vector child_errors = + ParseChildConfig(p.second, &child_config); + if (!child_errors.empty()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_VECTOR_AND_CPP_STRING( + absl::StrCat("field:targets key:", p.first), &child_errors)); } else { - target_map[p.first] = std::move(*config); + target_map[p.first] = std::move(child_config); } } } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("weighted_target_experimental LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (!error_list.empty()) { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "weighted_target_experimental LB policy config", &error_list); + return nullptr; } return MakeRefCounted(std::move(target_map)); } private: - static absl::StatusOr ParseChildConfig( - const Json& json) { + static std::vector ParseChildConfig( + const Json& json, WeightedTargetLbConfig::ChildConfig* child_config) { + std::vector error_list; if (json.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError("value should be of type object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "value should be of type object")); + return error_list; } - WeightedTargetLbConfig::ChildConfig child_config; - std::vector errors; // Weight. auto it = json.object_value().find("weight"); if (it == json.object_value().end()) { - errors.emplace_back("required field \"weight\" not specified"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "required field \"weight\" not specified")); } else if (it->second.type() != Json::Type::NUMBER) { - errors.emplace_back("field:weight error:must be of type number"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:weight error:must be of type number")); } else { int weight = gpr_parse_nonnegative_int(it->second.string_value().c_str()); if (weight == -1) { - errors.emplace_back("field:weight error:unparseable value"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:weight error:unparseable value")); } else if (weight == 0) { - errors.emplace_back( - "field:weight error:value must be greater than zero"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:weight error:value must be greater than zero")); } else { - child_config.weight = weight; + child_config->weight = weight; } } // Child policy. it = json.object_value().find("childPolicy"); if (it != json.object_value().end()) { - auto config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second); - if (!config.ok()) { - errors.emplace_back( - absl::StrCat("field:childPolicy: ", config.status().message())); - } else { - child_config.config = std::move(*config); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + child_config->config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second, + &parse_error); + if (child_config->config == nullptr) { + GPR_DEBUG_ASSERT(!GRPC_ERROR_IS_NONE(parse_error)); + std::vector child_errors; + child_errors.push_back(parse_error); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_VECTOR("field:childPolicy", &child_errors)); } } - // Return result. - if (!errors.empty()) { - return absl::InvalidArgumentError(absl::StrCat( - "errors parsing target config: [", absl::StrJoin(errors, "; "), "]")); - } - return child_config; + return error_list; } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc index 53329824e9d..e9813de64ef 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -28,7 +29,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -75,7 +75,7 @@ TraceFlag grpc_cds_lb_trace(false, "cds_lb"); namespace { -constexpr absl::string_view kCds = "cds_experimental"; +constexpr char kCds[] = "cds_experimental"; constexpr int kMaxAggregateClusterRecursionDepth = 16; @@ -84,7 +84,7 @@ class CdsLbConfig : public LoadBalancingPolicy::Config { public: explicit CdsLbConfig(std::string cluster) : cluster_(std::move(cluster)) {} const std::string& cluster() const { return cluster_; } - absl::string_view name() const override { return kCds; } + const char* name() const override { return kCds; } private: std::string cluster_; @@ -95,7 +95,7 @@ class CdsLb : public LoadBalancingPolicy { public: CdsLb(RefCountedPtr xds_client, Args args); - absl::string_view name() const override { return kCds; } + const char* name() const override { return kCds; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -517,9 +517,11 @@ void CdsLb::OnClusterChanged(const std::string& name, this, json_str.c_str()); } grpc_error_handle error = GRPC_ERROR_NONE; - auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json); - if (!config.ok()) { - OnError(name, absl::UnavailableError(config.status().message())); + RefCountedPtr config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json, &error); + if (!GRPC_ERROR_IS_NONE(error)) { + OnError(name, absl::UnavailableError(grpc_error_std_string(error))); + GRPC_ERROR_UNREF(error); return; } // Create child policy if not already present. @@ -529,7 +531,7 @@ void CdsLb::OnClusterChanged(const std::string& name, args.args = args_; args.channel_control_helper = absl::make_unique(Ref()); child_policy_ = LoadBalancingPolicyRegistry::CreateLoadBalancingPolicy( - (*config)->name(), std::move(args)); + config->name(), std::move(args)); if (child_policy_ == nullptr) { OnError(name, absl::UnavailableError("failed to create child policy")); return; @@ -538,12 +540,12 @@ void CdsLb::OnClusterChanged(const std::string& name, interested_parties()); if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) { gpr_log(GPR_INFO, "[cdslb %p] created child policy %s (%p)", this, - std::string((*config)->name()).c_str(), child_policy_.get()); + config->name(), child_policy_.get()); } } // Update child policy. UpdateArgs args; - args.config = std::move(*config); + args.config = std::move(config); if (xds_certificate_provider_ != nullptr) { args.args = args_.SetObject(xds_certificate_provider_); } else { @@ -723,32 +725,35 @@ class CdsLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(xds_client), std::move(args)); } - absl::string_view name() const override { return kCds; } + const char* name() const override { return kCds; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // xds was mentioned as a policy in the deprecated loadBalancingPolicy // field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:cds policy requires configuration. " "Please use loadBalancingConfig field of service config instead."); + return nullptr; } - std::vector errors; + std::vector error_list; // cluster name. std::string cluster; auto it = json.object_value().find("cluster"); if (it == json.object_value().end()) { - errors.emplace_back("required field 'cluster' not present"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "required field 'cluster' not present")); } else if (it->second.type() != Json::Type::STRING) { - errors.emplace_back("field:cluster error:type should be string"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:cluster error:type should be string")); } else { cluster = it->second.string_value(); } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("errors parsing CDS LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (!error_list.empty()) { + *error = GRPC_ERROR_CREATE_FROM_VECTOR("Cds Parser", &error_list); + return nullptr; } return MakeRefCounted(std::move(cluster)); } diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc index d524e2c5499..13d23aeaf69 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc @@ -32,7 +32,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "absl/types/variant.h" @@ -138,7 +137,7 @@ CircuitBreakerCallCounterMap::CallCounter::~CallCounter() { // LB policy // -constexpr absl::string_view kXdsClusterImpl = "xds_cluster_impl_experimental"; +constexpr char kXdsClusterImpl[] = "xds_cluster_impl_experimental"; // Config for xDS Cluster Impl LB policy. class XdsClusterImplLbConfig : public LoadBalancingPolicy::Config { @@ -156,7 +155,7 @@ class XdsClusterImplLbConfig : public LoadBalancingPolicy::Config { max_concurrent_requests_(max_concurrent_requests), drop_config_(std::move(drop_config)) {} - absl::string_view name() const override { return kXdsClusterImpl; } + const char* name() const override { return kXdsClusterImpl; } RefCountedPtr child_policy() const { return child_policy_; @@ -186,7 +185,7 @@ class XdsClusterImplLb : public LoadBalancingPolicy { public: XdsClusterImplLb(RefCountedPtr xds_client, Args args); - absl::string_view name() const override { return kXdsClusterImpl; } + const char* name() const override { return kXdsClusterImpl; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -704,41 +703,48 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { std::move(args)); } - absl::string_view name() const override { return kXdsClusterImpl; } + const char* name() const override { return kXdsClusterImpl; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // This policy was configured in the deprecated loadBalancingPolicy // field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:xds_cluster_impl policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); + return nullptr; } - std::vector errors; + std::vector error_list; // Child policy. RefCountedPtr child_policy; auto it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - errors.emplace_back("field:childPolicy error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:childPolicy error:required field missing")); } else { - auto config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second); - if (!config.ok()) { - errors.emplace_back(absl::StrCat("field:childPolicy error:", - config.status().message())); - } else { - child_policy = std::move(*config); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + child_policy = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + it->second, &parse_error); + if (child_policy == nullptr) { + GPR_DEBUG_ASSERT(!GRPC_ERROR_IS_NONE(parse_error)); + std::vector child_errors; + child_errors.push_back(parse_error); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_VECTOR("field:childPolicy", &child_errors)); } } // Cluster name. std::string cluster_name; it = json.object_value().find("clusterName"); if (it == json.object_value().end()) { - errors.emplace_back("field:clusterName error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:clusterName error:required field missing")); } else if (it->second.type() != Json::Type::STRING) { - errors.emplace_back("field:clusterName error:type should be string"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:clusterName error:type should be string")); } else { cluster_name = it->second.string_value(); } @@ -747,7 +753,8 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("edsServiceName"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::STRING) { - errors.emplace_back("field:edsServiceName error:type should be string"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:edsServiceName error:type should be string")); } else { eds_service_name = it->second.string_value(); } @@ -757,17 +764,16 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("lrsLoadReportingServer"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back( - "field:lrsLoadReportingServer error:type should be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:lrsLoadReportingServer error:type should be object")); } else { grpc_error_handle parser_error; lrs_load_reporting_server = XdsBootstrap::XdsServer::Parse( it->second.object_value(), &parser_error); if (!GRPC_ERROR_IS_NONE(parser_error)) { - errors.emplace_back( - absl::StrCat("error parsing lrs_load_reporting_server: ", - grpc_error_std_string(parser_error))); - GRPC_ERROR_UNREF(parser_error); + error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( + absl::StrCat("errors parsing lrs_load_reporting_server"))); + error_list.push_back(parser_error); } } } @@ -776,8 +782,8 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("maxConcurrentRequests"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::NUMBER) { - errors.emplace_back( - "field:max_concurrent_requests error:must be of type number"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:max_concurrent_requests error:must be of type number")); } else { max_concurrent_requests = gpr_parse_nonnegative_int(it->second.string_value().c_str()); @@ -787,15 +793,20 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { auto drop_config = MakeRefCounted(); it = json.object_value().find("dropCategories"); if (it == json.object_value().end()) { - errors.emplace_back("field:dropCategories error:required field missing"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:dropCategories error:required field missing")); } else { - absl::Status status = ParseDropCategories(it->second, drop_config.get()); - if (!status.ok()) errors.emplace_back(status.message()); + std::vector child_errors = + ParseDropCategories(it->second, drop_config.get()); + if (!child_errors.empty()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_VECTOR( + "field:dropCategories", &child_errors)); + } } - if (!errors.empty()) { - return absl::InvalidArgumentError(absl::StrCat( - "errors parseing xds_cluster_impl_experimental LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (!error_list.empty()) { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "xds_cluster_impl_experimental LB policy config", &error_list); + return nullptr; } return MakeRefCounted( std::move(child_policy), std::move(cluster_name), @@ -804,59 +815,65 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { } private: - static absl::Status ParseDropCategories( + static std::vector ParseDropCategories( const Json& json, XdsEndpointResource::DropConfig* drop_config) { + std::vector error_list; if (json.type() != Json::Type::ARRAY) { - return absl::InvalidArgumentError("dropCategories field is not an array"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "dropCategories field is not an array")); + return error_list; } - std::vector errors; for (size_t i = 0; i < json.array_value().size(); ++i) { const Json& entry = json.array_value()[i]; - absl::Status status = ParseDropCategory(entry, drop_config); - if (!status.ok()) { - errors.emplace_back( - absl::StrCat("error parsing index ", i, ": ", status.message())); + std::vector child_errors = + ParseDropCategory(entry, drop_config); + if (!child_errors.empty()) { + grpc_error_handle error = GRPC_ERROR_CREATE_FROM_CPP_STRING( + absl::StrCat("errors parsing index ", i)); + for (size_t i = 0; i < child_errors.size(); ++i) { + error = grpc_error_add_child(error, child_errors[i]); + } + error_list.push_back(error); } } - if (!errors.empty()) { - return absl::InvalidArgumentError( - absl::StrCat("errors parsing dropCategories field: [", - absl::StrJoin(errors, "; "), "]")); - } - return absl::OkStatus(); + return error_list; } - static absl::Status ParseDropCategory( + static std::vector ParseDropCategory( const Json& json, XdsEndpointResource::DropConfig* drop_config) { + std::vector error_list; if (json.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError( - "dropCategories entry is not an object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "dropCategories entry is not an object")); + return error_list; } - std::vector errors; std::string category; auto it = json.object_value().find("category"); if (it == json.object_value().end()) { - errors.emplace_back("\"category\" field not present"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"category\" field not present")); } else if (it->second.type() != Json::Type::STRING) { - errors.emplace_back("\"category\" field is not a string"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"category\" field is not a string")); } else { category = it->second.string_value(); } uint32_t requests_per_million = 0; it = json.object_value().find("requests_per_million"); if (it == json.object_value().end()) { - errors.emplace_back("\"requests_per_million\" field is not present"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"requests_per_million\" field is not present")); } else if (it->second.type() != Json::Type::NUMBER) { - errors.emplace_back("\"requests_per_million\" field is not a number"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"requests_per_million\" field is not a number")); } else { requests_per_million = gpr_parse_nonnegative_int(it->second.string_value().c_str()); } - if (!errors.empty()) { - return absl::InvalidArgumentError(absl::StrJoin(errors, "; ")); + if (error_list.empty()) { + drop_config->AddCategory(std::move(category), requests_per_million); } - drop_config->AddCategory(std::move(category), requests_per_million); - return absl::OkStatus(); + return error_list; } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc index a5fd9573d87..daa07d156ef 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_manager.cc @@ -30,7 +30,6 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" -#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include @@ -68,8 +67,7 @@ TraceFlag grpc_xds_cluster_manager_lb_trace(false, "xds_cluster_manager_lb"); namespace { -constexpr absl::string_view kXdsClusterManager = - "xds_cluster_manager_experimental"; +constexpr char kXdsClusterManager[] = "xds_cluster_manager_experimental"; // Config for xds_cluster_manager LB policy. class XdsClusterManagerLbConfig : public LoadBalancingPolicy::Config { @@ -80,7 +78,7 @@ class XdsClusterManagerLbConfig : public LoadBalancingPolicy::Config { explicit XdsClusterManagerLbConfig(ClusterMap cluster_map) : cluster_map_(std::move(cluster_map)) {} - absl::string_view name() const override { return kXdsClusterManager; } + const char* name() const override { return kXdsClusterManager; } const ClusterMap& cluster_map() const { return cluster_map_; } @@ -93,7 +91,7 @@ class XdsClusterManagerLb : public LoadBalancingPolicy { public: explicit XdsClusterManagerLb(Args args); - absl::string_view name() const override { return kXdsClusterManager; } + const char* name() const override { return kXdsClusterManager; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -624,80 +622,89 @@ class XdsClusterManagerLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kXdsClusterManager; } + const char* name() const override { return kXdsClusterManager; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // xds_cluster_manager was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:xds_cluster_manager policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); + return nullptr; } - std::vector errors; + std::vector error_list; XdsClusterManagerLbConfig::ClusterMap cluster_map; std::set clusters_to_be_used; auto it = json.object_value().find("children"); if (it == json.object_value().end()) { - errors.emplace_back("field:children error:required field not present"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:children error:required field not present")); } else if (it->second.type() != Json::Type::OBJECT) { - errors.emplace_back("field:children error:type should be object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:children error:type should be object")); } else { for (const auto& p : it->second.object_value()) { const std::string& child_name = p.first; if (child_name.empty()) { - errors.emplace_back("field:children error: name cannot be empty"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "field:children element error: name cannot be empty")); continue; } - auto config = ParseChildConfig(p.second); - if (!config.ok()) { - errors.emplace_back( - absl::StrCat("field:children name:", child_name, - " error:", config.status().message())); + RefCountedPtr child_config; + std::vector child_errors = + ParseChildConfig(p.second, &child_config); + if (!child_errors.empty()) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_VECTOR_AND_CPP_STRING( + absl::StrCat("field:children name:", child_name), &child_errors)); } else { - cluster_map[child_name] = std::move(*config); + cluster_map[child_name] = std::move(child_config); clusters_to_be_used.insert(child_name); } } } if (cluster_map.empty()) { - errors.emplace_back("no valid children configured"); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("no valid children configured")); } - if (!errors.empty()) { - return absl::InvalidArgumentError(absl::StrCat( - "errors parsing xds_cluster_manager_experimental LB policy config: [", - absl::StrJoin(errors, "; "), "]")); + if (!error_list.empty()) { + *error = GRPC_ERROR_CREATE_FROM_VECTOR( + "xds_cluster_manager_experimental LB policy config", &error_list); + return nullptr; } return MakeRefCounted(std::move(cluster_map)); } private: - static absl::StatusOr> - ParseChildConfig(const Json& json) { + static std::vector ParseChildConfig( + const Json& json, + RefCountedPtr* child_config) { + std::vector error_list; if (json.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError("value should be of type object"); + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "value should be of type object")); + return error_list; } - RefCountedPtr child_config; - std::vector errors; auto it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - errors.emplace_back("did not find childPolicy"); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("did not find childPolicy")); } else { - auto config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second); - if (!config.ok()) { - errors.emplace_back(absl::StrCat("field:childPolicy error:", - config.status().message())); - } else { - child_config = std::move(*config); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + *child_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + it->second, &parse_error); + if (*child_config == nullptr) { + GPR_DEBUG_ASSERT(!GRPC_ERROR_IS_NONE(parse_error)); + std::vector child_errors; + child_errors.push_back(parse_error); + error_list.push_back( + GRPC_ERROR_CREATE_FROM_VECTOR("field:childPolicy", &child_errors)); } } - if (!errors.empty()) { - return absl::InvalidArgumentError(absl::StrJoin(errors, "; ")); - } - return child_config; + return error_list; } }; diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc index 9a0fdaf323e..1268d8e0553 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc @@ -72,7 +72,6 @@ #include "src/core/lib/resolver/resolver_registry.h" #include "src/core/lib/resolver/server_address.h" #include "src/core/lib/transport/connectivity_state.h" -#include "src/core/lib/transport/error_utils.h" #define GRPC_EDS_DEFAULT_FALLBACK_TIMEOUT 10000 @@ -84,8 +83,7 @@ const char* kXdsLocalityNameAttributeKey = "xds_locality_name"; namespace { -constexpr absl::string_view kXdsClusterResolver = - "xds_cluster_resolver_experimental"; +constexpr char kXdsClusterResolver[] = "xds_cluster_resolver_experimental"; // Config for EDS LB policy. class XdsClusterResolverLbConfig : public LoadBalancingPolicy::Config { @@ -119,8 +117,7 @@ class XdsClusterResolverLbConfig : public LoadBalancingPolicy::Config { : discovery_mechanisms_(std::move(discovery_mechanisms)), xds_lb_policy_(std::move(xds_lb_policy)) {} - absl::string_view name() const override { return kXdsClusterResolver; } - + const char* name() const override { return kXdsClusterResolver; } const std::vector& discovery_mechanisms() const { return discovery_mechanisms_; } @@ -137,7 +134,7 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { public: XdsClusterResolverLb(RefCountedPtr xds_client, Args args); - absl::string_view name() const override { return kXdsClusterResolver; } + const char* name() const override { return kXdsClusterResolver; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -979,15 +976,17 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { "[xds_cluster_resolver_lb %p] generated config for child policy: %s", this, json_str.c_str()); } - auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json); - if (!config.ok()) { + grpc_error_handle error = GRPC_ERROR_NONE; + RefCountedPtr config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json, &error); + if (!GRPC_ERROR_IS_NONE(error)) { // This should never happen, but if it does, we basically have no // way to fix it, so we put the channel in TRANSIENT_FAILURE. gpr_log(GPR_ERROR, "[xds_cluster_resolver_lb %p] error parsing generated child policy " "config -- " "will put channel in TRANSIENT_FAILURE: %s", - this, config.status().ToString().c_str()); + this, grpc_error_std_string(error).c_str()); absl::Status status = absl::InternalError( "xds_cluster_resolver LB policy: error parsing generated child policy " "config"); @@ -996,7 +995,7 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { absl::make_unique(status)); return nullptr; } - return std::move(*config); + return config; } void XdsClusterResolverLb::UpdateChildPolicyLocked() { @@ -1072,17 +1071,19 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { std::move(args)); } - absl::string_view name() const override { return kXdsClusterResolver; } + const char* name() const override { return kXdsClusterResolver; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); if (json.type() == Json::Type::JSON_NULL) { // xds_cluster_resolver was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - return absl::InvalidArgumentError( + *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:loadBalancingPolicy error:xds_cluster_resolver policy " "requires configuration. " "Please use loadBalancingConfig field of service config instead."); + return nullptr; } std::vector error_list; std::vector @@ -1144,11 +1145,10 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { policy_it = policy.find("RING_HASH"); if (policy_it != policy.end()) { xds_lb_policy = array[i]; - auto config = ParseRingHashLbConfig(policy_it->second); - if (!config.ok()) { - error_list.emplace_back( - absl_status_to_grpc_error(config.status())); - } + size_t min_ring_size; + size_t max_ring_size; + ParseRingHashLbConfig(policy_it->second, &min_ring_size, + &max_ring_size, &error_list); } } } @@ -1158,11 +1158,9 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { return MakeRefCounted( std::move(discovery_mechanisms), std::move(xds_lb_policy)); } else { - grpc_error_handle error = GRPC_ERROR_CREATE_FROM_VECTOR( + *error = GRPC_ERROR_CREATE_FROM_VECTOR( "xds_cluster_resolver_experimental LB policy config", &error_list); - absl::Status status = grpc_error_to_absl_status(error); - GRPC_ERROR_UNREF(error); - return status; + return nullptr; } } @@ -1302,8 +1300,7 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { } OrphanablePtr CreateLoadBalancingPolicy( - absl::string_view /*name*/, - LoadBalancingPolicy::Args args) const override { + const char* /*name*/, LoadBalancingPolicy::Args args) const override { return MakeOrphanable( xds_client_->Ref(DEBUG_LOCATION, "XdsClusterResolverLb"), std::move(args)); diff --git a/src/core/ext/filters/client_channel/lb_policy_factory.h b/src/core/ext/filters/client_channel/lb_policy_factory.h index 2631c5ed077..56932586a00 100644 --- a/src/core/ext/filters/client_channel/lb_policy_factory.h +++ b/src/core/ext/filters/client_channel/lb_policy_factory.h @@ -1,30 +1,30 @@ -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ #ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_FACTORY_H #define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_FACTORY_H #include -#include "absl/status/statusor.h" -#include "absl/strings/string_view.h" - #include "src/core/ext/filters/client_channel/lb_policy.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" namespace grpc_core { @@ -38,12 +38,13 @@ class LoadBalancingPolicyFactory { LoadBalancingPolicy::Args) const = 0; /// Returns the LB policy name that this factory provides. - virtual absl::string_view name() const = 0; + /// Caller does NOT take ownership of result. + virtual const char* name() const = 0; - virtual absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const = 0; + virtual RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const = 0; }; } // namespace grpc_core -#endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_FACTORY_H +#endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_FACTORY_H */ 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 af3884a5fcf..520ae5dbb53 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.cc +++ b/src/core/ext/filters/client_channel/lb_policy_registry.cc @@ -1,30 +1,33 @@ -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ #include #include "src/core/ext/filters/client_channel/lb_policy_registry.h" +#include + #include #include #include #include #include -#include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" @@ -38,24 +41,30 @@ namespace { class RegistryState { public: + RegistryState() {} + void RegisterLoadBalancingPolicyFactory( std::unique_ptr factory) { gpr_log(GPR_DEBUG, "registering LB policy factory for \"%s\"", - std::string(factory->name()).c_str()); - GPR_ASSERT(factories_.find(factory->name()) == factories_.end()); - factories_.emplace(factory->name(), std::move(factory)); + factory->name()); + for (size_t i = 0; i < factories_.size(); ++i) { + GPR_ASSERT(strcmp(factories_[i]->name(), factory->name()) != 0); + } + factories_.push_back(std::move(factory)); } LoadBalancingPolicyFactory* GetLoadBalancingPolicyFactory( absl::string_view name) const { - auto it = factories_.find(name); - if (it == factories_.end()) return nullptr; - return it->second.get(); + for (size_t i = 0; i < factories_.size(); ++i) { + if (name == factories_[i]->name()) { + return factories_[i].get(); + } + } + return nullptr; } private: - std::map> - factories_; + std::vector> factories_; }; RegistryState* g_state = nullptr; @@ -87,7 +96,7 @@ void LoadBalancingPolicyRegistry::Builder::RegisterLoadBalancingPolicyFactory( OrphanablePtr LoadBalancingPolicyRegistry::CreateLoadBalancingPolicy( - absl::string_view name, LoadBalancingPolicy::Args args) { + const char* name, LoadBalancingPolicy::Args args) { GPR_ASSERT(g_state != nullptr); // Find factory. LoadBalancingPolicyFactory* factory = @@ -101,11 +110,15 @@ bool LoadBalancingPolicyRegistry::LoadBalancingPolicyExists( absl::string_view name, bool* requires_config) { GPR_ASSERT(g_state != nullptr); auto* factory = g_state->GetLoadBalancingPolicyFactory(name); - if (factory == nullptr) return false; - // If requested, check if the load balancing policy allows an empty config. + if (factory == nullptr) { + return false; + } if (requires_config != nullptr) { - auto config = factory->ParseLoadBalancingConfig(Json()); - *requires_config = !config.ok(); + grpc_error_handle error = GRPC_ERROR_NONE; + // Check if the load balancing policy allows an empty config + *requires_config = + factory->ParseLoadBalancingConfig(Json(), &error) == nullptr; + GRPC_ERROR_UNREF(error); } return true; } @@ -114,54 +127,64 @@ namespace { // Returns the JSON node of policy (with both policy name and config content) // given the JSON node of a LoadBalancingConfig array. -absl::StatusOr ParseLoadBalancingConfigHelper( - const Json& lb_config_array) { +grpc_error_handle ParseLoadBalancingConfigHelper( + const Json& lb_config_array, Json::Object::const_iterator* result) { if (lb_config_array.type() != Json::Type::ARRAY) { - return absl::InvalidArgumentError("type should be array"); + return GRPC_ERROR_CREATE_FROM_STATIC_STRING("type should be array"); } // Find the first LB policy that this client supports. std::vector policies_tried; for (const Json& lb_config : lb_config_array.array_value()) { if (lb_config.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError("child entry should be of type object"); + return GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "child entry should be of type object"); } if (lb_config.object_value().empty()) { - return absl::InvalidArgumentError("no policy found in child entry"); + return GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "no policy found in child entry"); } if (lb_config.object_value().size() > 1) { - return absl::InvalidArgumentError("oneOf violation"); + return GRPC_ERROR_CREATE_FROM_STATIC_STRING("oneOf violation"); } auto it = lb_config.object_value().begin(); if (it->second.type() != Json::Type::OBJECT) { - return absl::InvalidArgumentError("child entry should be of type object"); + return GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "child entry should be of type object"); } // If we support this policy, then select it. if (LoadBalancingPolicyRegistry::LoadBalancingPolicyExists( it->first.c_str(), nullptr)) { - return it; + *result = it; + return GRPC_ERROR_NONE; } policies_tried.push_back(it->first); } - return absl::FailedPreconditionError(absl::StrCat( + return GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( "No known policies in list: ", absl::StrJoin(policies_tried, " "))); } } // namespace -absl::StatusOr> -LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(const Json& json) { +RefCountedPtr +LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) { + GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); GPR_ASSERT(g_state != nullptr); - auto policy = ParseLoadBalancingConfigHelper(json); - if (!policy.ok()) return policy.status(); + Json::Object::const_iterator policy; + *error = ParseLoadBalancingConfigHelper(json, &policy); + if (!GRPC_ERROR_IS_NONE(*error)) { + return nullptr; + } // Find factory. LoadBalancingPolicyFactory* factory = - g_state->GetLoadBalancingPolicyFactory((*policy)->first.c_str()); + g_state->GetLoadBalancingPolicyFactory(policy->first.c_str()); if (factory == nullptr) { - return absl::FailedPreconditionError(absl::StrFormat( - "Factory not found for policy \"%s\"", (*policy)->first)); + *error = GRPC_ERROR_CREATE_FROM_CPP_STRING( + absl::StrFormat("Factory not found for policy \"%s\"", policy->first)); + return nullptr; } // Parse load balancing config via factory. - return factory->ParseLoadBalancingConfig((*policy)->second); + return factory->ParseLoadBalancingConfig(policy->second, error); } } // namespace grpc_core 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 23830fb96fb..dbfc94a35e8 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.h +++ b/src/core/ext/filters/client_channel/lb_policy_registry.h @@ -1,18 +1,20 @@ -// -// Copyright 2015 gRPC authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ #ifndef GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_REGISTRY_H #define GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_REGISTRY_H @@ -21,13 +23,13 @@ #include -#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "src/core/ext/filters/client_channel/lb_policy.h" #include "src/core/ext/filters/client_channel/lb_policy_factory.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" namespace grpc_core { @@ -51,7 +53,7 @@ class LoadBalancingPolicyRegistry { /// Creates an LB policy of the type specified by \a name. static OrphanablePtr CreateLoadBalancingPolicy( - absl::string_view name, LoadBalancingPolicy::Args args); + const char* name, LoadBalancingPolicy::Args args); /// Returns true if the LB policy factory specified by \a name exists in this /// registry. If the load balancing policy requires a config to be specified @@ -61,10 +63,10 @@ class LoadBalancingPolicyRegistry { /// Returns a parsed object of the load balancing policy to be used from a /// LoadBalancingConfig array \a json. - static absl::StatusOr> - ParseLoadBalancingConfig(const Json& json); + static RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error); }; } // namespace grpc_core -#endif // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_REGISTRY_H +#endif /* GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_REGISTRY_H */ 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 1e042424f7c..de813e2c333 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -25,8 +25,6 @@ #include #include "absl/memory/memory.h" -#include "absl/status/status.h" -#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/types/optional.h" @@ -90,13 +88,14 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const ChannelArgs& /*args*/, RefCountedPtr parsed_lb_config; auto it = json.object_value().find("loadBalancingConfig"); if (it != json.object_value().end()) { - auto config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(it->second); - if (!config.ok()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( - "field:loadBalancingConfig error:", config.status().message()))); - } else { - parsed_lb_config = std::move(*config); + grpc_error_handle parse_error = GRPC_ERROR_NONE; + parsed_lb_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + it->second, &parse_error); + if (!GRPC_ERROR_IS_NONE(parse_error)) { + std::vector lb_errors; + lb_errors.push_back(parse_error); + error_list.push_back(GRPC_ERROR_CREATE_FROM_VECTOR( + "field:loadBalancingConfig", &lb_errors)); } } // Parse deprecated LB policy. diff --git a/src/core/ext/xds/xds_cluster_specifier_plugin.cc b/src/core/ext/xds/xds_cluster_specifier_plugin.cc index 0d6a61e50bc..8af2046892e 100644 --- a/src/core/ext/xds/xds_cluster_specifier_plugin.cc +++ b/src/core/ext/xds/xds_cluster_specifier_plugin.cc @@ -99,13 +99,15 @@ XdsRouteLookupClusterSpecifierPlugin::GenerateLoadBalancingPolicyConfig( // somehow such that we automatically validate the resulting config against // the gRPC LB policy registry instead of requiring each plugin to do that // itself. - auto config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(lb_policy_config); - if (!config.ok()) { - return absl::InvalidArgumentError(absl::StrCat( + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(lb_policy_config, + &parse_error); + if (!GRPC_ERROR_IS_NONE(parse_error)) { + absl::Status status = absl::InvalidArgumentError(absl::StrCat( kXdsRouteLookupClusterSpecifierPluginConfigName, " ClusterSpecifierPlugin returned invalid LB policy config: ", - config.status().message())); + grpc_error_std_string(parse_error))); + GRPC_ERROR_UNREF(parse_error); + return status; } return lb_policy_config.Dump(); } diff --git a/test/core/client_channel/rls_lb_config_parser_test.cc b/test/core/client_channel/rls_lb_config_parser_test.cc index a4b4645d34a..edd76e7b5e6 100644 --- a/test/core/client_channel/rls_lb_config_parser_test.cc +++ b/test/core/client_channel/rls_lb_config_parser_test.cc @@ -161,12 +161,12 @@ TEST_F(RlsConfigParsingTest, InvalidChildPolicyConfig) { grpc_error_handle error = GRPC_ERROR_NONE; auto service_config = ServiceConfigImpl::Create(ChannelArgs(), service_config_json, &error); - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "errors parsing RLS LB policy config" CHILD_ERROR_TAG - "field:childPolicy" CHILD_ERROR_TAG - "errors parsing grpclb LB policy config: \\[" - "error parsing childPolicy field: type should be array\\]")); + EXPECT_THAT( + grpc_error_std_string(error), + ::testing::ContainsRegex( + "errors parsing RLS LB policy config" CHILD_ERROR_TAG + "field:childPolicy" CHILD_ERROR_TAG "GrpcLb Parser" CHILD_ERROR_TAG + "field:childPolicy" CHILD_ERROR_TAG "type should be array")); GRPC_ERROR_UNREF(error); } diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index c630113f618..284ba6a324c 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -534,7 +534,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigPickFirst) { static_cast( svc_cfg->GetGlobalParsedConfig(0)); auto lb_config = parsed_config->parsed_lb_config(); - EXPECT_EQ(lb_config->name(), "pick_first"); + EXPECT_STREQ(lb_config->name(), "pick_first"); } TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigRoundRobin) { @@ -546,7 +546,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigRoundRobin) { auto parsed_config = static_cast( svc_cfg->GetGlobalParsedConfig(0)); auto lb_config = parsed_config->parsed_lb_config(); - EXPECT_EQ(lb_config->name(), "round_robin"); + EXPECT_STREQ(lb_config->name(), "round_robin"); } TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigGrpclb) { @@ -560,7 +560,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigGrpclb) { static_cast( svc_cfg->GetGlobalParsedConfig(0)); auto lb_config = parsed_config->parsed_lb_config(); - EXPECT_EQ(lb_config->name(), "grpclb"); + EXPECT_STREQ(lb_config->name(), "grpclb"); } TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigXds) { @@ -583,7 +583,7 @@ TEST_F(ClientChannelParserTest, ValidLoadBalancingConfigXds) { static_cast( svc_cfg->GetGlobalParsedConfig(0)); auto lb_config = parsed_config->parsed_lb_config(); - EXPECT_EQ(lb_config->name(), "xds_cluster_resolver_experimental"); + EXPECT_STREQ(lb_config->name(), "xds_cluster_resolver_experimental"); } TEST_F(ClientChannelParserTest, UnknownLoadBalancingConfig) { @@ -595,8 +595,8 @@ TEST_F(ClientChannelParserTest, UnknownLoadBalancingConfig) { ::testing::ContainsRegex("Service config parsing error" CHILD_ERROR_TAG "Global Params" CHILD_ERROR_TAG "Client channel global parser" CHILD_ERROR_TAG - "field:loadBalancingConfig " - "error:No known policies in list: unknown")); + "field:loadBalancingConfig" CHILD_ERROR_TAG + "No known policies in list: unknown")); GRPC_ERROR_UNREF(error); } @@ -613,9 +613,9 @@ TEST_F(ClientChannelParserTest, InvalidGrpclbLoadBalancingConfig) { "Service config parsing error" CHILD_ERROR_TAG "Global Params" CHILD_ERROR_TAG "Client channel global parser" CHILD_ERROR_TAG - "field:loadBalancingConfig error:" - "errors parsing grpclb LB policy config: \\[" - "error parsing childPolicy field: type should be array\\]")); + "field:loadBalancingConfig" CHILD_ERROR_TAG + "GrpcLb Parser" CHILD_ERROR_TAG + "field:childPolicy" CHILD_ERROR_TAG "type should be array")); GRPC_ERROR_UNREF(error); } diff --git a/test/core/end2end/tests/retry_lb_drop.cc b/test/core/end2end/tests/retry_lb_drop.cc index b30c6242402..5d2480c1fb4 100644 --- a/test/core/end2end/tests/retry_lb_drop.cc +++ b/test/core/end2end/tests/retry_lb_drop.cc @@ -49,13 +49,13 @@ namespace grpc_core { namespace { -constexpr absl::string_view kDropPolicyName = "drop_lb"; +const char* kDropPolicyName = "drop_lb"; class DropPolicy : public LoadBalancingPolicy { public: explicit DropPolicy(Args args) : LoadBalancingPolicy(std::move(args)) {} - absl::string_view name() const override { return kDropPolicyName; } + const char* name() const override { return kDropPolicyName; } void UpdateLocked(UpdateArgs) override { channel_control_helper()->UpdateState(GRPC_CHANNEL_READY, absl::Status(), @@ -77,7 +77,7 @@ class DropPolicy : public LoadBalancingPolicy { class DropLbConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kDropPolicyName; } + const char* name() const override { return kDropPolicyName; } }; class DropPolicyFactory : public LoadBalancingPolicyFactory { @@ -87,10 +87,10 @@ class DropPolicyFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kDropPolicyName; } + const char* name() const override { return kDropPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } }; diff --git a/test/core/end2end/tests/retry_lb_fail.cc b/test/core/end2end/tests/retry_lb_fail.cc index dea7b6d3584..6bc512fca7b 100644 --- a/test/core/end2end/tests/retry_lb_fail.cc +++ b/test/core/end2end/tests/retry_lb_fail.cc @@ -47,7 +47,7 @@ namespace grpc_core { namespace { -constexpr absl::string_view kFailPolicyName = "fail_lb"; +const char* kFailPolicyName = "fail_lb"; std::atomic g_num_lb_picks; @@ -55,7 +55,7 @@ class FailPolicy : public LoadBalancingPolicy { public: explicit FailPolicy(Args args) : LoadBalancingPolicy(std::move(args)) {} - absl::string_view name() const override { return kFailPolicyName; } + const char* name() const override { return kFailPolicyName; } void UpdateLocked(UpdateArgs) override { absl::Status status = absl::AbortedError("LB pick failed"); @@ -84,7 +84,7 @@ class FailPolicy : public LoadBalancingPolicy { class FailLbConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kFailPolicyName; } + const char* name() const override { return kFailPolicyName; } }; class FailPolicyFactory : public LoadBalancingPolicyFactory { @@ -94,10 +94,10 @@ class FailPolicyFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kFailPolicyName; } + const char* name() const override { return kFailPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } }; diff --git a/test/core/util/test_lb_policies.cc b/test/core/util/test_lb_policies.cc index 734f5c7bfd7..a39cc92bedb 100644 --- a/test/core/util/test_lb_policies.cc +++ b/test/core/util/test_lb_policies.cc @@ -60,7 +60,7 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { public: ForwardingLoadBalancingPolicy( std::unique_ptr delegating_helper, Args args, - absl::string_view delegate_policy_name, intptr_t initial_refcount = 1) + const char* delegate_policy_name, intptr_t initial_refcount = 1) : LoadBalancingPolicy(std::move(args), initial_refcount) { Args delegate_args; delegate_args.work_serializer = work_serializer(); @@ -92,12 +92,12 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { // TestPickArgsLb // -constexpr absl::string_view kTestPickArgsLbPolicyName = "test_pick_args_lb"; +constexpr char kTestPickArgsLbPolicyName[] = "test_pick_args_lb"; class TestPickArgsLb : public ForwardingLoadBalancingPolicy { public: TestPickArgsLb(Args args, TestPickArgsCallback cb, - absl::string_view delegate_policy_name) + const char* delegate_policy_name) : ForwardingLoadBalancingPolicy( absl::make_unique(RefCountedPtr(this), cb), std::move(args), delegate_policy_name, @@ -105,7 +105,7 @@ class TestPickArgsLb : public ForwardingLoadBalancingPolicy { ~TestPickArgsLb() override = default; - absl::string_view name() const override { return kTestPickArgsLbPolicyName; } + const char* name() const override { return kTestPickArgsLbPolicyName; } private: class Picker : public SubchannelPicker { @@ -167,13 +167,13 @@ class TestPickArgsLb : public ForwardingLoadBalancingPolicy { class TestPickArgsLbConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kTestPickArgsLbPolicyName; } + const char* name() const override { return kTestPickArgsLbPolicyName; } }; class TestPickArgsLbFactory : public LoadBalancingPolicyFactory { public: explicit TestPickArgsLbFactory(TestPickArgsCallback cb, - absl::string_view delegate_policy_name) + const char* delegate_policy_name) : cb_(std::move(cb)), delegate_policy_name_(delegate_policy_name) {} OrphanablePtr CreateLoadBalancingPolicy( @@ -182,16 +182,16 @@ class TestPickArgsLbFactory : public LoadBalancingPolicyFactory { delegate_policy_name_); } - absl::string_view name() const override { return kTestPickArgsLbPolicyName; } + const char* name() const override { return kTestPickArgsLbPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } private: TestPickArgsCallback cb_; - std::string delegate_policy_name_; + const char* delegate_policy_name_; }; // @@ -217,7 +217,7 @@ class InterceptRecvTrailingMetadataLoadBalancingPolicy ~InterceptRecvTrailingMetadataLoadBalancingPolicy() override = default; - absl::string_view name() const override { + const char* name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } @@ -305,7 +305,7 @@ class InterceptRecvTrailingMetadataLoadBalancingPolicy class InterceptTrailingConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { + const char* name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } }; @@ -321,12 +321,12 @@ class InterceptTrailingFactory : public LoadBalancingPolicyFactory { std::move(args), cb_); } - absl::string_view name() const override { + const char* name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } @@ -353,7 +353,7 @@ class AddressTestLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { ~AddressTestLoadBalancingPolicy() override = default; - absl::string_view name() const override { return kAddressTestLbPolicyName; } + const char* name() const override { return kAddressTestLbPolicyName; } private: class Helper : public ChannelControlHelper { @@ -396,7 +396,7 @@ class AddressTestLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { class AddressTestConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { return kAddressTestLbPolicyName; } + const char* name() const override { return kAddressTestLbPolicyName; } }; class AddressTestFactory : public LoadBalancingPolicyFactory { @@ -408,10 +408,10 @@ class AddressTestFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args), cb_); } - absl::string_view name() const override { return kAddressTestLbPolicyName; } + const char* name() const override { return kAddressTestLbPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } @@ -430,7 +430,7 @@ class FixedAddressConfig : public LoadBalancingPolicy::Config { explicit FixedAddressConfig(std::string address) : address_(std::move(address)) {} - absl::string_view name() const override { return kFixedAddressLbPolicyName; } + const char* name() const override { return kFixedAddressLbPolicyName; } const std::string& address() const { return address_; } @@ -450,7 +450,7 @@ class FixedAddressLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { ~FixedAddressLoadBalancingPolicy() override = default; - absl::string_view name() const override { return kFixedAddressLbPolicyName; } + const char* name() const override { return kFixedAddressLbPolicyName; } void UpdateLocked(UpdateArgs args) override { auto* config = static_cast(args.config.get()); @@ -517,20 +517,17 @@ class FixedAddressFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - absl::string_view name() const override { return kFixedAddressLbPolicyName; } + const char* name() const override { return kFixedAddressLbPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& json, grpc_error_handle* error) const override { std::vector error_list; std::string address; ParseJsonObjectField(json.object_value(), "address", &address, &error_list); if (!error_list.empty()) { - grpc_error_handle error = GRPC_ERROR_CREATE_FROM_VECTOR( + *error = GRPC_ERROR_CREATE_FROM_VECTOR( "errors parsing fixed_address_lb config", &error_list); - absl::Status status = - absl::InvalidArgumentError(grpc_error_std_string(error)); - GRPC_ERROR_UNREF(error); - return status; + return nullptr; } return MakeRefCounted(std::move(address)); } @@ -545,7 +542,7 @@ constexpr char kOobBackendMetricTestLbPolicyName[] = class OobBackendMetricTestConfig : public LoadBalancingPolicy::Config { public: - absl::string_view name() const override { + const char* name() const override { return kOobBackendMetricTestLbPolicyName; } }; @@ -565,7 +562,7 @@ class OobBackendMetricTestLoadBalancingPolicy ~OobBackendMetricTestLoadBalancingPolicy() override = default; - absl::string_view name() const override { + const char* name() const override { return kOobBackendMetricTestLbPolicyName; } @@ -641,12 +638,12 @@ class OobBackendMetricTestFactory : public LoadBalancingPolicyFactory { std::move(args), cb_); } - absl::string_view name() const override { + const char* name() const override { return kOobBackendMetricTestLbPolicyName; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /*json*/, grpc_error_handle* /*error*/) const override { return MakeRefCounted(); } @@ -656,8 +653,8 @@ class OobBackendMetricTestFactory : public LoadBalancingPolicyFactory { } // namespace -void RegisterTestPickArgsLoadBalancingPolicy( - TestPickArgsCallback cb, absl::string_view delegate_policy_name) { +void RegisterTestPickArgsLoadBalancingPolicy(TestPickArgsCallback cb, + const char* delegate_policy_name) { LoadBalancingPolicyRegistry::Builder::RegisterLoadBalancingPolicyFactory( absl::make_unique(std::move(cb), delegate_policy_name)); diff --git a/test/core/util/test_lb_policies.h b/test/core/util/test_lb_policies.h index 24133c900a6..b30006b3dc7 100644 --- a/test/core/util/test_lb_policies.h +++ b/test/core/util/test_lb_policies.h @@ -41,8 +41,7 @@ using TestPickArgsCallback = std::function; // Registers an LB policy called "test_pick_args_lb" that passes the args // passed to SubchannelPicker::Pick() to cb. void RegisterTestPickArgsLoadBalancingPolicy( - TestPickArgsCallback cb, - absl::string_view delegate_policy_name = "pick_first"); + TestPickArgsCallback cb, const char* delegate_policy_name = "pick_first"); struct TrailingMetadataArgsSeen { absl::Status status; diff --git a/test/core/xds/xds_lb_policy_registry_test.cc b/test/core/xds/xds_lb_policy_registry_test.cc index 38ecedbe62c..72e8bd41f8b 100644 --- a/test/core/xds/xds_lb_policy_registry_test.cc +++ b/test/core/xds/xds_lb_policy_registry_test.cc @@ -274,10 +274,10 @@ class CustomLbPolicyFactory : public LoadBalancingPolicyFactory { return nullptr; } - absl::string_view name() const override { return "test.CustomLb"; } + const char* name() const override { return "test.CustomLb"; } - absl::StatusOr> - ParseLoadBalancingConfig(const Json& /*json*/) const override { + RefCountedPtr ParseLoadBalancingConfig( + const Json& /* json */, grpc_error_handle* /* error */) const override { return nullptr; } };