diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 12aaf43fb0b..30c96331012 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1135,9 +1135,8 @@ RefCountedPtr ChooseLbPolicy( Json config_json = Json::Array{Json::Object{ {std::string(*policy_name), Json::Object{}}, }}; - grpc_error_handle parse_error = GRPC_ERROR_NONE; - auto lb_policy_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - config_json, &parse_error); + auto lb_policy_config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(config_json); // The policy name came from one of three places: // - The deprecated loadBalancingPolicy field in the service config, // in which case the code in ClientChannelServiceConfigParser @@ -1147,9 +1146,8 @@ 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 != nullptr); - GPR_ASSERT(GRPC_ERROR_IS_NONE(parse_error)); - return lb_policy_config; + GPR_ASSERT(lb_policy_config.ok()); + return std::move(*lb_policy_config); } } // namespace @@ -1248,9 +1246,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), - lb_policy_config->name()); + UpdateServiceConfigInControlPlaneLocked( + std::move(service_config), std::move(config_selector), + std::string(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 689a409c2c2..3be781d8b91 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -306,7 +306,7 @@ class LoadBalancingPolicy : public InternallyRefCounted { ~Config() override = default; // Returns the load balancing policy name - virtual const char* name() const = 0; + virtual absl::string_view name() const = 0; }; /// Data passed to the UpdateLocked() method when new addresses and @@ -350,7 +350,7 @@ class LoadBalancingPolicy : public InternallyRefCounted { LoadBalancingPolicy& operator=(const LoadBalancingPolicy&) = delete; /// Returns the name of the LB policy. - virtual const char* name() const = 0; + virtual absl::string_view 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 bc3a4ed37a5..d6d687ee591 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,7 +18,6 @@ #include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h" -#include #include #include @@ -230,7 +229,8 @@ 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 ", args.config->name()); + child_policy_ == nullptr ? "" : "pending ", + std::string(args.config->name()).c_str()); } auto& lb_policy = child_policy_ == nullptr ? child_policy_ : pending_child_policy_; @@ -274,7 +274,7 @@ void ChildPolicyHandler::ResetBackoffLocked() { } OrphanablePtr ChildPolicyHandler::CreateChildPolicy( - const char* child_policy_name, const ChannelArgs& args) { + absl::string_view 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,14 +284,15 @@ 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\"", child_policy_name); + gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", + std::string(child_policy_name).c_str()); 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, - child_policy_name, lb_policy.get()); + std::string(child_policy_name).c_str(), lb_policy.get()); } channel_control_helper()->AddTraceEvent( ChannelControlHelper::TRACE_INFO, @@ -304,12 +305,12 @@ OrphanablePtr ChildPolicyHandler::CreateChildPolicy( bool ChildPolicyHandler::ConfigChangeRequiresNewPolicyInstance( LoadBalancingPolicy::Config* old_config, LoadBalancingPolicy::Config* new_config) const { - return strcmp(old_config->name(), new_config->name()) != 0; + return old_config->name() != new_config->name(); } OrphanablePtr ChildPolicyHandler::CreateLoadBalancingPolicy( - const char* name, LoadBalancingPolicy::Args args) const { + absl::string_view 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 d3093518e02..9b7c4096f94 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,11 +16,12 @@ #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" @@ -40,7 +41,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { ChildPolicyHandler(Args args, TraceFlag* tracer) : LoadBalancingPolicy(std::move(args)), tracer_(tracer) {} - const char* name() const override { return "child_policy_handler"; } + absl::string_view name() const override { return "child_policy_handler"; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -56,7 +57,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { // May be overridden by subclasses to avoid recursion when an LB // policy factory returns a ChildPolicyHandler. virtual OrphanablePtr CreateLoadBalancingPolicy( - const char* name, LoadBalancingPolicy::Args args) const; + absl::string_view name, LoadBalancingPolicy::Args args) const; private: class Helper; @@ -64,7 +65,7 @@ class ChildPolicyHandler : public LoadBalancingPolicy { void ShutdownLocked() override; OrphanablePtr CreateChildPolicy( - const char* child_policy_name, const ChannelArgs& args); + absl::string_view 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 3570fe46450..c7e01a7b29a 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 char kGrpclb[] = "grpclb"; +constexpr absl::string_view kGrpclb = "grpclb"; class GrpcLbConfig : public LoadBalancingPolicy::Config { public: @@ -168,7 +168,8 @@ class GrpcLbConfig : public LoadBalancingPolicy::Config { std::string service_name) : child_policy_(std::move(child_policy)), service_name_(std::move(service_name)) {} - const char* name() const override { return kGrpclb; } + + absl::string_view name() const override { return kGrpclb; } RefCountedPtr child_policy() const { return child_policy_; @@ -185,7 +186,7 @@ class GrpcLb : public LoadBalancingPolicy { public: explicit GrpcLb(Args args); - const char* name() const override { return kGrpclb; } + absl::string_view name() const override { return kGrpclb; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -1845,28 +1846,27 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kGrpclb; } + absl::string_view name() const override { return kGrpclb; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { return MakeRefCounted(nullptr, ""); } - std::vector error_list; - Json child_policy_config_json_tmp; - const Json* child_policy_config_json; + std::vector error_list; 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.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:serviceName error:type should be string")); + error_list.emplace_back( + "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,25 +1876,24 @@ class GrpcLbFactory : public LoadBalancingPolicyFactory { } else { child_policy_config_json = &it->second; } - grpc_error_handle parse_error = GRPC_ERROR_NONE; - RefCountedPtr child_policy_config = + auto child_policy_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - *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)); + *child_policy_config_json); + if (!child_policy_config.ok()) { + error_list.emplace_back( + absl::StrCat("error parsing childPolicy field: ", + child_policy_config.status().message())); } if (error_list.empty()) { - return MakeRefCounted(std::move(child_policy_config), + return MakeRefCounted(std::move(*child_policy_config), std::move(service_name)); } else { - *error = GRPC_ERROR_CREATE_FROM_VECTOR("GrpcLb Parser", &error_list); - return nullptr; + return absl::InvalidArgumentError( + absl::StrCat("errors parsing grpclb LB policy config: [", + absl::StrJoin(error_list, "; "), "]")); } } -}; // 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 2b6ab6096a2..4d91935a030 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,6 +36,8 @@ #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" @@ -84,7 +86,8 @@ bool XdsOutlierDetectionEnabled() { namespace { -constexpr char kOutlierDetection[] = "outlier_detection_experimental"; +constexpr absl::string_view kOutlierDetection = + "outlier_detection_experimental"; // Config for xDS Cluster Impl LB policy. class OutlierDetectionLbConfig : public LoadBalancingPolicy::Config { @@ -95,7 +98,7 @@ class OutlierDetectionLbConfig : public LoadBalancingPolicy::Config { : outlier_detection_config_(outlier_detection_config), child_policy_(std::move(child_policy)) {} - const char* name() const override { return kOutlierDetection; } + absl::string_view name() const override { return kOutlierDetection; } bool CountingEnabled() const { return ( @@ -122,7 +125,7 @@ class OutlierDetectionLb : public LoadBalancingPolicy { public: explicit OutlierDetectionLb(Args args); - const char* name() const override { return kOutlierDetection; } + absl::string_view name() const override { return kOutlierDetection; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -1008,28 +1011,27 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kOutlierDetection; } + absl::string_view name() const override { return kOutlierDetection; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { // This policy was configured in the deprecated loadBalancingPolicy // field or in the client API. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "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) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:successRateEjection error:type must be object")); + errors.emplace_back( + "field:successRateEjection error:type must be object"); } else { OutlierDetectionConfig::SuccessRateEjection success_config; const Json::Object& object = it->second.object_value(); @@ -1051,8 +1053,8 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("failurePercentageEjection"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:successRateEjection error:type must be object")); + errors.emplace_back( + "field:successRateEjection error:type must be object"); } else { OutlierDetectionConfig::FailurePercentageEjection failure_config; const Json::Object& object = it->second.object_value(); @@ -1089,24 +1091,26 @@ class OutlierDetectionLbFactory : public LoadBalancingPolicyFactory { RefCountedPtr child_policy; it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:childPolicy error:required field missing")); + errors.emplace_back("field:childPolicy error:required field missing"); } else { - 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)); + 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); } } - if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( - "outlier_detection_experimental LB policy config", &error_list); - return nullptr; + 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, "; "), "]")); } 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 3435948a434..923d8ee0f49 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,6 +29,7 @@ #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,7 +46,6 @@ #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 char kPickFirst[] = "pick_first"; +constexpr absl::string_view kPickFirst = "pick_first"; class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(Args args); - const char* name() const override { return kPickFirst; } + absl::string_view 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: - const char* name() const override { return kPickFirst; } + absl::string_view name() const override { return kPickFirst; } }; // @@ -516,10 +516,10 @@ class PickFirstFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kPickFirst; } + absl::string_view name() const override { return kPickFirst; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) 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 97640431145..4b5e7f92959 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,6 +30,7 @@ #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" @@ -66,7 +67,7 @@ TraceFlag grpc_lb_priority_trace(false, "priority_lb"); namespace { -constexpr char kPriority[] = "priority_experimental"; +constexpr absl::string_view 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 @@ -89,7 +90,7 @@ class PriorityLbConfig : public LoadBalancingPolicy::Config { std::vector priorities) : children_(std::move(children)), priorities_(std::move(priorities)) {} - const char* name() const override { return kPriority; } + absl::string_view name() const override { return kPriority; } const std::map& children() const { return children_; @@ -106,7 +107,7 @@ class PriorityLb : public LoadBalancingPolicy { public: explicit PriorityLb(Args args); - const char* name() const override { return kPriority; } + absl::string_view name() const override { return kPriority; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -926,49 +927,40 @@ class PriorityLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kPriority; } + absl::string_view name() const override { return kPriority; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { // priority was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "field:loadBalancingPolicy error:priority policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); - return nullptr; } - std::vector error_list; + std::vector errors; // Children. std::map children; auto it = json.object_value().find("children"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:children error:required field missing")); + errors.emplace_back("field:children error:required field missing"); } else if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:children error:type should be object")); + errors.emplace_back("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) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( - absl::StrCat("field:children key:", child_name, - " error:should be type object"))); + errors.emplace_back(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()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( - absl::StrCat("field:children key:", child_name, - " error:missing 'config' field"))); + errors.emplace_back(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. @@ -978,23 +970,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) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( + errors.emplace_back( absl::StrCat("field:children key:", child_name, " field:ignore_reresolution_requests:should " - "be type boolean"))); + "be type boolean")); } } - 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); + 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; } - children[child_name].config = std::move(config); - children[child_name].ignore_reresolution_requests = - ignore_resolution_requests; } } } @@ -1003,40 +995,37 @@ class PriorityLbFactory : public LoadBalancingPolicyFactory { std::vector priorities; it = json.object_value().find("priorities"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:priorities error:required field missing")); + errors.emplace_back("field:priorities error:required field missing"); } else if (it->second.type() != Json::Type::ARRAY) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:priorities error:type should be array")); + errors.emplace_back("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) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( - "field:priorities element:", i, " error:should be type string"))); + errors.emplace_back(absl::StrCat("field:priorities element:", i, + " error:should be type string")); } else if (children.find(element.string_value()) == children.end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( - "field:priorities element:", i, " error:unknown child '", - element.string_value(), "'"))); + errors.emplace_back(absl::StrCat("field:priorities element:", i, + " error:unknown child '", + element.string_value(), "'")); } else { priorities.emplace_back(element.string_value()); } } if (priorities.size() != children.size()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( + errors.emplace_back(absl::StrCat( "field:priorities error:priorities size (", priorities.size(), - ") != children size (", children.size(), ")"))); + ") != children size (", children.size(), ")")); } } - 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; + if (!errors.empty()) { + return absl::InvalidArgumentError( + absl::StrCat("priority_experimental LB policy config: [", + absl::StrJoin(errors, "; "), "]")); } + 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 fd25d8f4163..6d16c8e18be 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,6 +16,8 @@ #include +#include "src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h" + #include #include @@ -37,11 +39,10 @@ #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,6 +56,7 @@ #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" @@ -81,56 +83,60 @@ UniqueTypeName RequestHashAttributeName() { } // Helper Parser method -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; +absl::StatusOr ParseRingHashLbConfig(const Json& json) { if (json.type() != Json::Type::OBJECT) { - error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "ring_hash_experimental should be of type object")); - return; + return absl::InvalidArgumentError( + "ring_hash_experimental should be of type object"); } + 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) { - error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:min_ring_size error: should be of type number")); + errors.emplace_back( + "field:min_ring_size error: should be of type number"); } else { - *min_ring_size = gpr_parse_nonnegative_int( + config.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) { - error_list->push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:max_ring_size error: should be of type number")); + errors.emplace_back( + "field:max_ring_size error: should be of type number"); } else { - *max_ring_size = gpr_parse_nonnegative_int( + config.max_ring_size = gpr_parse_nonnegative_int( ring_hash_it->second.string_value().c_str()); } } - 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( + 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( "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 char kRingHash[] = "ring_hash_experimental"; +constexpr absl::string_view 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) {} - const char* name() const override { return kRingHash; } + absl::string_view 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_; } @@ -147,7 +153,7 @@ class RingHash : public LoadBalancingPolicy { public: explicit RingHash(Args args); - const char* name() const override { return kRingHash; } + absl::string_view name() const override { return kRingHash; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -875,21 +881,14 @@ class RingHashFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kRingHash; } + absl::string_view name() const override { return kRingHash; } - 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; - } + 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); } }; 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 fdc0b10278b..89ddab0b831 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,10 +21,9 @@ #include -#include +#include "absl/status/statusor.h" #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 { @@ -33,9 +32,11 @@ UniqueTypeName RequestHashAttributeName(); // Helper Parsing method to parse ring hash policy configs; for example, ring // hash size validity. -void ParseRingHashLbConfig(const Json& json, size_t* min_ring_size, - size_t* max_ring_size, - std::vector* error_list); +struct RingHashConfig { + size_t min_ring_size = 1024; + size_t max_ring_size = 8388608; +}; +absl::StatusOr ParseRingHashLbConfig(const Json& json); } // 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 94a57cfe215..9bd016b6200 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 { -const char* kRls = "rls_experimental"; +constexpr absl::string_view 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)) {} - const char* name() const override { return kRls; } + absl::string_view 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); - const char* name() const override { return kRls; } + absl::string_view 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()); } - pending_config_ = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - child_policy_config, &error); + auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + child_policy_config); // Returned RLS target fails the validation. - if (!GRPC_ERROR_IS_NONE(error)) { + if (!config.ok()) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_rls_trace)) { gpr_log(GPR_INFO, "[rlslb %p] ChildPolicyWrapper=%p [%s]: config failed to parse: " - "%s; config: %s", + "%s", lb_policy_.get(), this, target_.c_str(), - grpc_error_std_string(error).c_str(), - child_policy_config.Dump().c_str()); + config.status().ToString().c_str()); } pending_config_.reset(); picker_ = absl::make_unique( - grpc_error_to_absl_status(error)); - GRPC_ERROR_UNREF(error); + absl::UnavailableError(config.status().message())); child_policy_.reset(); + } else { + pending_config_ = std::move(*config); } } @@ -2441,43 +2441,42 @@ 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. - RefCountedPtr parsed_config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - *child_policy_config, &error); - if (!GRPC_ERROR_IS_NONE(error)) return error; + auto parsed_config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( + *child_policy_config); + if (!parsed_config.ok()) { + return absl_status_to_grpc_error(parsed_config.status()); + } // 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. - 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; - } + 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: - const char* name() const override { return kRls; } + absl::string_view name() const override { return kRls; } OrphanablePtr CreateLoadBalancingPolicy( LoadBalancingPolicy::Args args) const override { return MakeOrphanable(std::move(args)); } - RefCountedPtr ParseLoadBalancingConfig( - const Json& config, grpc_error_handle* error) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& config) const override { std::vector error_list; // Parse routeLookupConfig. RlsLbConfig::RouteLookupConfig route_lookup_config; @@ -2542,8 +2541,13 @@ class RlsLbFactory : public LoadBalancingPolicyFactory { } } // Return result. - *error = GRPC_ERROR_CREATE_FROM_VECTOR( - "errors parsing RLS LB policy config", &error_list); + 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); + } 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 c10f8975463..da960fb6b95 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,6 +29,7 @@ #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 @@ -44,7 +45,6 @@ #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 char kRoundRobin[] = "round_robin"; +constexpr absl::string_view kRoundRobin = "round_robin"; class RoundRobin : public LoadBalancingPolicy { public: explicit RoundRobin(Args args); - const char* name() const override { return kRoundRobin; } + absl::string_view 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: - const char* name() const override { return kRoundRobin; } + absl::string_view name() const override { return kRoundRobin; } }; class RoundRobinFactory : public LoadBalancingPolicyFactory { @@ -509,10 +509,10 @@ class RoundRobinFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kRoundRobin; } + absl::string_view name() const override { return kRoundRobin; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) 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 cfc0feb840e..2cf75212224 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,6 +30,7 @@ #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" @@ -53,7 +54,6 @@ #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 char kWeightedTarget[] = "weighted_target_experimental"; +constexpr absl::string_view 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)) {} - const char* name() const override { return kWeightedTarget; } + absl::string_view 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); - const char* name() const override { return kWeightedTarget; } + absl::string_view name() const override { return kWeightedTarget; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -683,96 +683,87 @@ class WeightedTargetLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kWeightedTarget; } + absl::string_view name() const override { return kWeightedTarget; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { // weighted_target was mentioned as a policy in the deprecated // loadBalancingPolicy field or in the client API. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "field:loadBalancingPolicy error:weighted_target policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); - return nullptr; } - std::vector error_list; + std::vector errors; // Weight map. WeightedTargetLbConfig::TargetMap target_map; auto it = json.object_value().find("targets"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:targets error:required field not present")); + errors.emplace_back("field:targets error:required field not present"); } else if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:targets error:type should be object")); + errors.emplace_back("field:targets error:type should be object"); } else { for (const auto& p : it->second.object_value()) { - 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)); + auto config = ParseChildConfig(p.second); + if (!config.ok()) { + errors.emplace_back(config.status().message()); } else { - target_map[p.first] = std::move(child_config); + target_map[p.first] = std::move(*config); } } } - if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( - "weighted_target_experimental LB policy config", &error_list); - return nullptr; + if (!errors.empty()) { + return absl::InvalidArgumentError( + absl::StrCat("weighted_target_experimental LB policy config: [", + absl::StrJoin(errors, "; "), "]")); } return MakeRefCounted(std::move(target_map)); } private: - static std::vector ParseChildConfig( - const Json& json, WeightedTargetLbConfig::ChildConfig* child_config) { - std::vector error_list; + static absl::StatusOr ParseChildConfig( + const Json& json) { if (json.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "value should be of type object")); - return error_list; + return absl::InvalidArgumentError("value should be of type object"); } + WeightedTargetLbConfig::ChildConfig child_config; + std::vector errors; // Weight. auto it = json.object_value().find("weight"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "required field \"weight\" not specified")); + errors.emplace_back("required field \"weight\" not specified"); } else if (it->second.type() != Json::Type::NUMBER) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:weight error:must be of type number")); + errors.emplace_back("field:weight error:must be of type number"); } else { int weight = gpr_parse_nonnegative_int(it->second.string_value().c_str()); if (weight == -1) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:weight error:unparseable value")); + errors.emplace_back("field:weight error:unparseable value"); } else if (weight == 0) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:weight error:value must be greater than zero")); + errors.emplace_back( + "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()) { - 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)); + 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); } } - return error_list; + // Return result. + if (!errors.empty()) { + return absl::InvalidArgumentError(absl::StrCat( + "errors parsing target config: [", absl::StrJoin(errors, "; "), "]")); + } + return child_config; } }; 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 e9813de64ef..53329824e9d 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,7 +21,6 @@ #include #include #include -#include #include #include @@ -29,6 +28,7 @@ #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 char kCds[] = "cds_experimental"; +constexpr absl::string_view 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_; } - const char* name() const override { return kCds; } + absl::string_view name() const override { return kCds; } private: std::string cluster_; @@ -95,7 +95,7 @@ class CdsLb : public LoadBalancingPolicy { public: CdsLb(RefCountedPtr xds_client, Args args); - const char* name() const override { return kCds; } + absl::string_view name() const override { return kCds; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -517,11 +517,9 @@ void CdsLb::OnClusterChanged(const std::string& name, this, json_str.c_str()); } grpc_error_handle error = GRPC_ERROR_NONE; - 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); + auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json); + if (!config.ok()) { + OnError(name, absl::UnavailableError(config.status().message())); return; } // Create child policy if not already present. @@ -531,7 +529,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; @@ -540,12 +538,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, - config->name(), child_policy_.get()); + std::string((*config)->name()).c_str(), 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 { @@ -725,35 +723,32 @@ class CdsLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(xds_client), std::move(args)); } - const char* name() const override { return kCds; } + absl::string_view name() const override { return kCds; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { // xds was mentioned as a policy in the deprecated loadBalancingPolicy // field or in the client API. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "field:loadBalancingPolicy error:cds policy requires configuration. " "Please use loadBalancingConfig field of service config instead."); - return nullptr; } - std::vector error_list; + std::vector errors; // cluster name. std::string cluster; auto it = json.object_value().find("cluster"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "required field 'cluster' not present")); + errors.emplace_back("required field 'cluster' not present"); } else if (it->second.type() != Json::Type::STRING) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:cluster error:type should be string")); + errors.emplace_back("field:cluster error:type should be string"); } else { cluster = it->second.string_value(); } - if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR("Cds Parser", &error_list); - return nullptr; + if (!errors.empty()) { + return absl::InvalidArgumentError( + absl::StrCat("errors parsing CDS LB policy config: [", + absl::StrJoin(errors, "; "), "]")); } 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 13d23aeaf69..d524e2c5499 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,6 +32,7 @@ #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" @@ -137,7 +138,7 @@ CircuitBreakerCallCounterMap::CallCounter::~CallCounter() { // LB policy // -constexpr char kXdsClusterImpl[] = "xds_cluster_impl_experimental"; +constexpr absl::string_view kXdsClusterImpl = "xds_cluster_impl_experimental"; // Config for xDS Cluster Impl LB policy. class XdsClusterImplLbConfig : public LoadBalancingPolicy::Config { @@ -155,7 +156,7 @@ class XdsClusterImplLbConfig : public LoadBalancingPolicy::Config { max_concurrent_requests_(max_concurrent_requests), drop_config_(std::move(drop_config)) {} - const char* name() const override { return kXdsClusterImpl; } + absl::string_view name() const override { return kXdsClusterImpl; } RefCountedPtr child_policy() const { return child_policy_; @@ -185,7 +186,7 @@ class XdsClusterImplLb : public LoadBalancingPolicy { public: XdsClusterImplLb(RefCountedPtr xds_client, Args args); - const char* name() const override { return kXdsClusterImpl; } + absl::string_view name() const override { return kXdsClusterImpl; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -703,48 +704,41 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { std::move(args)); } - const char* name() const override { return kXdsClusterImpl; } + absl::string_view name() const override { return kXdsClusterImpl; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { if (json.type() == Json::Type::JSON_NULL) { // This policy was configured in the deprecated loadBalancingPolicy // field or in the client API. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "field:loadBalancingPolicy error:xds_cluster_impl policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); - return nullptr; } - std::vector error_list; + std::vector errors; // Child policy. RefCountedPtr child_policy; auto it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:childPolicy error:required field missing")); + errors.emplace_back("field:childPolicy error:required field missing"); } else { - 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)); + 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); } } // Cluster name. std::string cluster_name; it = json.object_value().find("clusterName"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:clusterName error:required field missing")); + errors.emplace_back("field:clusterName error:required field missing"); } else if (it->second.type() != Json::Type::STRING) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:clusterName error:type should be string")); + errors.emplace_back("field:clusterName error:type should be string"); } else { cluster_name = it->second.string_value(); } @@ -753,8 +747,7 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("edsServiceName"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::STRING) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:edsServiceName error:type should be string")); + errors.emplace_back("field:edsServiceName error:type should be string"); } else { eds_service_name = it->second.string_value(); } @@ -764,16 +757,17 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("lrsLoadReportingServer"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:lrsLoadReportingServer error:type should be object")); + errors.emplace_back( + "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)) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_CPP_STRING( - absl::StrCat("errors parsing lrs_load_reporting_server"))); - error_list.push_back(parser_error); + errors.emplace_back( + absl::StrCat("error parsing lrs_load_reporting_server: ", + grpc_error_std_string(parser_error))); + GRPC_ERROR_UNREF(parser_error); } } } @@ -782,8 +776,8 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { it = json.object_value().find("maxConcurrentRequests"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::NUMBER) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:max_concurrent_requests error:must be of type number")); + errors.emplace_back( + "field:max_concurrent_requests error:must be of type number"); } else { max_concurrent_requests = gpr_parse_nonnegative_int(it->second.string_value().c_str()); @@ -793,20 +787,15 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { auto drop_config = MakeRefCounted(); it = json.object_value().find("dropCategories"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:dropCategories error:required field missing")); + errors.emplace_back("field:dropCategories error:required field missing"); } else { - 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)); - } + absl::Status status = ParseDropCategories(it->second, drop_config.get()); + if (!status.ok()) errors.emplace_back(status.message()); } - if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( - "xds_cluster_impl_experimental LB policy config", &error_list); - return nullptr; + if (!errors.empty()) { + return absl::InvalidArgumentError(absl::StrCat( + "errors parseing xds_cluster_impl_experimental LB policy config: [", + absl::StrJoin(errors, "; "), "]")); } return MakeRefCounted( std::move(child_policy), std::move(cluster_name), @@ -815,65 +804,59 @@ class XdsClusterImplLbFactory : public LoadBalancingPolicyFactory { } private: - static std::vector ParseDropCategories( + static absl::Status ParseDropCategories( const Json& json, XdsEndpointResource::DropConfig* drop_config) { - std::vector error_list; if (json.type() != Json::Type::ARRAY) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "dropCategories field is not an array")); - return error_list; + return absl::InvalidArgumentError("dropCategories field is not an array"); } + std::vector errors; for (size_t i = 0; i < json.array_value().size(); ++i) { const Json& entry = json.array_value()[i]; - 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); + absl::Status status = ParseDropCategory(entry, drop_config); + if (!status.ok()) { + errors.emplace_back( + absl::StrCat("error parsing index ", i, ": ", status.message())); } } - return error_list; + if (!errors.empty()) { + return absl::InvalidArgumentError( + absl::StrCat("errors parsing dropCategories field: [", + absl::StrJoin(errors, "; "), "]")); + } + return absl::OkStatus(); } - static std::vector ParseDropCategory( + static absl::Status ParseDropCategory( const Json& json, XdsEndpointResource::DropConfig* drop_config) { - std::vector error_list; if (json.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "dropCategories entry is not an object")); - return error_list; + return absl::InvalidArgumentError( + "dropCategories entry is not an object"); } + std::vector errors; std::string category; auto it = json.object_value().find("category"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"category\" field not present")); + errors.emplace_back("\"category\" field not present"); } else if (it->second.type() != Json::Type::STRING) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"category\" field is not a string")); + errors.emplace_back("\"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()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"requests_per_million\" field is not present")); + errors.emplace_back("\"requests_per_million\" field is not present"); } else if (it->second.type() != Json::Type::NUMBER) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"requests_per_million\" field is not a number")); + errors.emplace_back("\"requests_per_million\" field is not a number"); } else { requests_per_million = gpr_parse_nonnegative_int(it->second.string_value().c_str()); } - if (error_list.empty()) { - drop_config->AddCategory(std::move(category), requests_per_million); + if (!errors.empty()) { + return absl::InvalidArgumentError(absl::StrJoin(errors, "; ")); } - return error_list; + drop_config->AddCategory(std::move(category), requests_per_million); + return absl::OkStatus(); } }; 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 daa07d156ef..a5fd9573d87 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,6 +30,7 @@ #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 @@ -67,7 +68,8 @@ TraceFlag grpc_xds_cluster_manager_lb_trace(false, "xds_cluster_manager_lb"); namespace { -constexpr char kXdsClusterManager[] = "xds_cluster_manager_experimental"; +constexpr absl::string_view kXdsClusterManager = + "xds_cluster_manager_experimental"; // Config for xds_cluster_manager LB policy. class XdsClusterManagerLbConfig : public LoadBalancingPolicy::Config { @@ -78,7 +80,7 @@ class XdsClusterManagerLbConfig : public LoadBalancingPolicy::Config { explicit XdsClusterManagerLbConfig(ClusterMap cluster_map) : cluster_map_(std::move(cluster_map)) {} - const char* name() const override { return kXdsClusterManager; } + absl::string_view name() const override { return kXdsClusterManager; } const ClusterMap& cluster_map() const { return cluster_map_; } @@ -91,7 +93,7 @@ class XdsClusterManagerLb : public LoadBalancingPolicy { public: explicit XdsClusterManagerLb(Args args); - const char* name() const override { return kXdsClusterManager; } + absl::string_view name() const override { return kXdsClusterManager; } void UpdateLocked(UpdateArgs args) override; void ExitIdleLocked() override; @@ -622,89 +624,80 @@ class XdsClusterManagerLbFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kXdsClusterManager; } + absl::string_view name() const override { return kXdsClusterManager; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { 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. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "field:loadBalancingPolicy error:xds_cluster_manager policy requires " "configuration. Please use loadBalancingConfig field of service " "config instead."); - return nullptr; } - std::vector error_list; + std::vector errors; XdsClusterManagerLbConfig::ClusterMap cluster_map; std::set clusters_to_be_used; auto it = json.object_value().find("children"); if (it == json.object_value().end()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:children error:required field not present")); + errors.emplace_back("field:children error:required field not present"); } else if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:children error:type should be object")); + errors.emplace_back("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()) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "field:children element error: name cannot be empty")); + errors.emplace_back("field:children error: name cannot be empty"); continue; } - 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)); + auto config = ParseChildConfig(p.second); + if (!config.ok()) { + errors.emplace_back( + absl::StrCat("field:children name:", child_name, + " error:", config.status().message())); } else { - cluster_map[child_name] = std::move(child_config); + cluster_map[child_name] = std::move(*config); clusters_to_be_used.insert(child_name); } } } if (cluster_map.empty()) { - error_list.push_back( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("no valid children configured")); + errors.emplace_back("no valid children configured"); } - if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( - "xds_cluster_manager_experimental LB policy config", &error_list); - return nullptr; + if (!errors.empty()) { + return absl::InvalidArgumentError(absl::StrCat( + "errors parsing xds_cluster_manager_experimental LB policy config: [", + absl::StrJoin(errors, "; "), "]")); } return MakeRefCounted(std::move(cluster_map)); } private: - static std::vector ParseChildConfig( - const Json& json, - RefCountedPtr* child_config) { - std::vector error_list; + static absl::StatusOr> + ParseChildConfig(const Json& json) { if (json.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "value should be of type object")); - return error_list; + return absl::InvalidArgumentError("value should be of type object"); } + RefCountedPtr child_config; + std::vector errors; auto it = json.object_value().find("childPolicy"); if (it == json.object_value().end()) { - error_list.push_back( - GRPC_ERROR_CREATE_FROM_STATIC_STRING("did not find childPolicy")); + errors.emplace_back("did not find childPolicy"); } else { - 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)); + 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); } } - return error_list; + if (!errors.empty()) { + return absl::InvalidArgumentError(absl::StrJoin(errors, "; ")); + } + return child_config; } }; 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 1268d8e0553..9a0fdaf323e 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,6 +72,7 @@ #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 @@ -83,7 +84,8 @@ const char* kXdsLocalityNameAttributeKey = "xds_locality_name"; namespace { -constexpr char kXdsClusterResolver[] = "xds_cluster_resolver_experimental"; +constexpr absl::string_view kXdsClusterResolver = + "xds_cluster_resolver_experimental"; // Config for EDS LB policy. class XdsClusterResolverLbConfig : public LoadBalancingPolicy::Config { @@ -117,7 +119,8 @@ class XdsClusterResolverLbConfig : public LoadBalancingPolicy::Config { : discovery_mechanisms_(std::move(discovery_mechanisms)), xds_lb_policy_(std::move(xds_lb_policy)) {} - const char* name() const override { return kXdsClusterResolver; } + absl::string_view name() const override { return kXdsClusterResolver; } + const std::vector& discovery_mechanisms() const { return discovery_mechanisms_; } @@ -134,7 +137,7 @@ class XdsClusterResolverLb : public LoadBalancingPolicy { public: XdsClusterResolverLb(RefCountedPtr xds_client, Args args); - const char* name() const override { return kXdsClusterResolver; } + absl::string_view name() const override { return kXdsClusterResolver; } void UpdateLocked(UpdateArgs args) override; void ResetBackoffLocked() override; @@ -976,17 +979,15 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { "[xds_cluster_resolver_lb %p] generated config for child policy: %s", this, json_str.c_str()); } - grpc_error_handle error = GRPC_ERROR_NONE; - RefCountedPtr config = - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json, &error); - if (!GRPC_ERROR_IS_NONE(error)) { + auto config = LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(json); + if (!config.ok()) { // 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, grpc_error_std_string(error).c_str()); + this, config.status().ToString().c_str()); absl::Status status = absl::InternalError( "xds_cluster_resolver LB policy: error parsing generated child policy " "config"); @@ -995,7 +996,7 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() { absl::make_unique(status)); return nullptr; } - return config; + return std::move(*config); } void XdsClusterResolverLb::UpdateChildPolicyLocked() { @@ -1071,19 +1072,17 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { std::move(args)); } - const char* name() const override { return kXdsClusterResolver; } + absl::string_view name() const override { return kXdsClusterResolver; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { 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. - *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + return absl::InvalidArgumentError( "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 @@ -1145,10 +1144,11 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { policy_it = policy.find("RING_HASH"); if (policy_it != policy.end()) { xds_lb_policy = array[i]; - size_t min_ring_size; - size_t max_ring_size; - ParseRingHashLbConfig(policy_it->second, &min_ring_size, - &max_ring_size, &error_list); + auto config = ParseRingHashLbConfig(policy_it->second); + if (!config.ok()) { + error_list.emplace_back( + absl_status_to_grpc_error(config.status())); + } } } } @@ -1158,9 +1158,11 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { return MakeRefCounted( std::move(discovery_mechanisms), std::move(xds_lb_policy)); } else { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( + grpc_error_handle error = GRPC_ERROR_CREATE_FROM_VECTOR( "xds_cluster_resolver_experimental LB policy config", &error_list); - return nullptr; + absl::Status status = grpc_error_to_absl_status(error); + GRPC_ERROR_UNREF(error); + return status; } } @@ -1300,7 +1302,8 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory { } OrphanablePtr CreateLoadBalancingPolicy( - const char* /*name*/, LoadBalancingPolicy::Args args) const override { + absl::string_view /*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 56932586a00..2631c5ed077 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,13 +38,12 @@ class LoadBalancingPolicyFactory { LoadBalancingPolicy::Args) const = 0; /// Returns the LB policy name that this factory provides. - /// Caller does NOT take ownership of result. - virtual const char* name() const = 0; + virtual absl::string_view name() const = 0; - virtual RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const = 0; + virtual absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) 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 520ae5dbb53..af3884a5fcf 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.cc +++ b/src/core/ext/filters/client_channel/lb_policy_registry.cc @@ -1,33 +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. +// #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" @@ -41,30 +38,24 @@ namespace { class RegistryState { public: - RegistryState() {} - void RegisterLoadBalancingPolicyFactory( std::unique_ptr factory) { gpr_log(GPR_DEBUG, "registering LB policy factory for \"%s\"", - 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)); + std::string(factory->name()).c_str()); + GPR_ASSERT(factories_.find(factory->name()) == factories_.end()); + factories_.emplace(factory->name(), std::move(factory)); } LoadBalancingPolicyFactory* GetLoadBalancingPolicyFactory( absl::string_view name) const { - for (size_t i = 0; i < factories_.size(); ++i) { - if (name == factories_[i]->name()) { - return factories_[i].get(); - } - } - return nullptr; + auto it = factories_.find(name); + if (it == factories_.end()) return nullptr; + return it->second.get(); } private: - std::vector> factories_; + std::map> + factories_; }; RegistryState* g_state = nullptr; @@ -96,7 +87,7 @@ void LoadBalancingPolicyRegistry::Builder::RegisterLoadBalancingPolicyFactory( OrphanablePtr LoadBalancingPolicyRegistry::CreateLoadBalancingPolicy( - const char* name, LoadBalancingPolicy::Args args) { + absl::string_view name, LoadBalancingPolicy::Args args) { GPR_ASSERT(g_state != nullptr); // Find factory. LoadBalancingPolicyFactory* factory = @@ -110,15 +101,11 @@ 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 (factory == nullptr) return false; + // If requested, check if the load balancing policy allows an empty config. if (requires_config != nullptr) { - 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); + auto config = factory->ParseLoadBalancingConfig(Json()); + *requires_config = !config.ok(); } return true; } @@ -127,64 +114,54 @@ namespace { // Returns the JSON node of policy (with both policy name and config content) // given the JSON node of a LoadBalancingConfig array. -grpc_error_handle ParseLoadBalancingConfigHelper( - const Json& lb_config_array, Json::Object::const_iterator* result) { +absl::StatusOr ParseLoadBalancingConfigHelper( + const Json& lb_config_array) { if (lb_config_array.type() != Json::Type::ARRAY) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING("type should be array"); + return absl::InvalidArgumentError("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 GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "child entry should be of type object"); + return absl::InvalidArgumentError("child entry should be of type object"); } if (lb_config.object_value().empty()) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "no policy found in child entry"); + return absl::InvalidArgumentError("no policy found in child entry"); } if (lb_config.object_value().size() > 1) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING("oneOf violation"); + return absl::InvalidArgumentError("oneOf violation"); } auto it = lb_config.object_value().begin(); if (it->second.type() != Json::Type::OBJECT) { - return GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "child entry should be of type object"); + return absl::InvalidArgumentError("child entry should be of type object"); } // If we support this policy, then select it. if (LoadBalancingPolicyRegistry::LoadBalancingPolicyExists( it->first.c_str(), nullptr)) { - *result = it; - return GRPC_ERROR_NONE; + return it; } policies_tried.push_back(it->first); } - return GRPC_ERROR_CREATE_FROM_CPP_STRING(absl::StrCat( + return absl::FailedPreconditionError(absl::StrCat( "No known policies in list: ", absl::StrJoin(policies_tried, " "))); } } // namespace -RefCountedPtr -LoadBalancingPolicyRegistry::ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) { - GPR_DEBUG_ASSERT(error != nullptr && GRPC_ERROR_IS_NONE(*error)); +absl::StatusOr> +LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(const Json& json) { GPR_ASSERT(g_state != nullptr); - Json::Object::const_iterator policy; - *error = ParseLoadBalancingConfigHelper(json, &policy); - if (!GRPC_ERROR_IS_NONE(*error)) { - return nullptr; - } + auto policy = ParseLoadBalancingConfigHelper(json); + if (!policy.ok()) return policy.status(); // Find factory. LoadBalancingPolicyFactory* factory = - g_state->GetLoadBalancingPolicyFactory(policy->first.c_str()); + g_state->GetLoadBalancingPolicyFactory((*policy)->first.c_str()); if (factory == nullptr) { - *error = GRPC_ERROR_CREATE_FROM_CPP_STRING( - absl::StrFormat("Factory not found for policy \"%s\"", policy->first)); - return nullptr; + return absl::FailedPreconditionError(absl::StrFormat( + "Factory not found for policy \"%s\"", (*policy)->first)); } // Parse load balancing config via factory. - return factory->ParseLoadBalancingConfig(policy->second, error); + return factory->ParseLoadBalancingConfig((*policy)->second); } } // 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 dbfc94a35e8..23830fb96fb 100644 --- a/src/core/ext/filters/client_channel/lb_policy_registry.h +++ b/src/core/ext/filters/client_channel/lb_policy_registry.h @@ -1,20 +1,18 @@ -/* - * - * 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 @@ -23,13 +21,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 { @@ -53,7 +51,7 @@ class LoadBalancingPolicyRegistry { /// Creates an LB policy of the type specified by \a name. static OrphanablePtr CreateLoadBalancingPolicy( - const char* name, LoadBalancingPolicy::Args args); + absl::string_view 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 @@ -63,10 +61,10 @@ class LoadBalancingPolicyRegistry { /// Returns a parsed object of the load balancing policy to be used from a /// LoadBalancingConfig array \a json. - static RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error); + static absl::StatusOr> + ParseLoadBalancingConfig(const Json& json); }; } // 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 de813e2c333..1e042424f7c 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -25,6 +25,8 @@ #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" @@ -88,14 +90,13 @@ ClientChannelServiceConfigParser::ParseGlobalParams(const ChannelArgs& /*args*/, RefCountedPtr parsed_lb_config; auto it = json.object_value().find("loadBalancingConfig"); if (it != json.object_value().end()) { - 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)); + 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); } } // 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 8af2046892e..0d6a61e50bc 100644 --- a/src/core/ext/xds/xds_cluster_specifier_plugin.cc +++ b/src/core/ext/xds/xds_cluster_specifier_plugin.cc @@ -99,15 +99,13 @@ 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. - LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(lb_policy_config, - &parse_error); - if (!GRPC_ERROR_IS_NONE(parse_error)) { - absl::Status status = absl::InvalidArgumentError(absl::StrCat( + auto config = + LoadBalancingPolicyRegistry::ParseLoadBalancingConfig(lb_policy_config); + if (!config.ok()) { + return absl::InvalidArgumentError(absl::StrCat( kXdsRouteLookupClusterSpecifierPluginConfigName, " ClusterSpecifierPlugin returned invalid LB policy config: ", - grpc_error_std_string(parse_error))); - GRPC_ERROR_UNREF(parse_error); - return status; + config.status().message())); } 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 edd76e7b5e6..a4b4645d34a 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 "GrpcLb Parser" CHILD_ERROR_TAG - "field:childPolicy" CHILD_ERROR_TAG "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 + "errors parsing grpclb LB policy config: \\[" + "error parsing childPolicy field: 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 284ba6a324c..c630113f618 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_STREQ(lb_config->name(), "pick_first"); + EXPECT_EQ(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_STREQ(lb_config->name(), "round_robin"); + EXPECT_EQ(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_STREQ(lb_config->name(), "grpclb"); + EXPECT_EQ(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_STREQ(lb_config->name(), "xds_cluster_resolver_experimental"); + EXPECT_EQ(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" CHILD_ERROR_TAG - "No known policies in list: unknown")); + "field:loadBalancingConfig " + "error: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" CHILD_ERROR_TAG - "GrpcLb Parser" CHILD_ERROR_TAG - "field:childPolicy" CHILD_ERROR_TAG "type should be array")); + "field:loadBalancingConfig error:" + "errors parsing grpclb LB policy config: \\[" + "error parsing childPolicy field: 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 35373aa88c7..7212f251810 100644 --- a/test/core/end2end/tests/retry_lb_drop.cc +++ b/test/core/end2end/tests/retry_lb_drop.cc @@ -37,13 +37,13 @@ namespace grpc_core { namespace { -const char* kDropPolicyName = "drop_lb"; +constexpr absl::string_view kDropPolicyName = "drop_lb"; class DropPolicy : public LoadBalancingPolicy { public: explicit DropPolicy(Args args) : LoadBalancingPolicy(std::move(args)) {} - const char* name() const override { return kDropPolicyName; } + absl::string_view name() const override { return kDropPolicyName; } void UpdateLocked(UpdateArgs) override { channel_control_helper()->UpdateState(GRPC_CHANNEL_READY, absl::Status(), @@ -65,7 +65,7 @@ class DropPolicy : public LoadBalancingPolicy { class DropLbConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { return kDropPolicyName; } + absl::string_view name() const override { return kDropPolicyName; } }; class DropPolicyFactory : public LoadBalancingPolicyFactory { @@ -75,10 +75,10 @@ class DropPolicyFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kDropPolicyName; } + absl::string_view name() const override { return kDropPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) 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 970667d4d9c..2e33317c2a0 100644 --- a/test/core/end2end/tests/retry_lb_fail.cc +++ b/test/core/end2end/tests/retry_lb_fail.cc @@ -37,7 +37,7 @@ namespace grpc_core { namespace { -const char* kFailPolicyName = "fail_lb"; +constexpr absl::string_view kFailPolicyName = "fail_lb"; std::atomic g_num_lb_picks; @@ -45,7 +45,7 @@ class FailPolicy : public LoadBalancingPolicy { public: explicit FailPolicy(Args args) : LoadBalancingPolicy(std::move(args)) {} - const char* name() const override { return kFailPolicyName; } + absl::string_view name() const override { return kFailPolicyName; } void UpdateLocked(UpdateArgs) override { absl::Status status = absl::AbortedError("LB pick failed"); @@ -74,7 +74,7 @@ class FailPolicy : public LoadBalancingPolicy { class FailLbConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { return kFailPolicyName; } + absl::string_view name() const override { return kFailPolicyName; } }; class FailPolicyFactory : public LoadBalancingPolicyFactory { @@ -84,10 +84,10 @@ class FailPolicyFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kFailPolicyName; } + absl::string_view name() const override { return kFailPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return MakeRefCounted(); } }; diff --git a/test/core/util/test_lb_policies.cc b/test/core/util/test_lb_policies.cc index 7d68152df72..aaabd02f3da 100644 --- a/test/core/util/test_lb_policies.cc +++ b/test/core/util/test_lb_policies.cc @@ -51,7 +51,7 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { public: ForwardingLoadBalancingPolicy( std::unique_ptr delegating_helper, Args args, - const char* delegate_policy_name, intptr_t initial_refcount = 1) + absl::string_view delegate_policy_name, intptr_t initial_refcount = 1) : LoadBalancingPolicy(std::move(args), initial_refcount) { Args delegate_args; delegate_args.work_serializer = work_serializer(); @@ -83,12 +83,12 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { // TestPickArgsLb // -constexpr char kTestPickArgsLbPolicyName[] = "test_pick_args_lb"; +constexpr absl::string_view kTestPickArgsLbPolicyName = "test_pick_args_lb"; class TestPickArgsLb : public ForwardingLoadBalancingPolicy { public: TestPickArgsLb(Args args, TestPickArgsCallback cb, - const char* delegate_policy_name) + absl::string_view delegate_policy_name) : ForwardingLoadBalancingPolicy( absl::make_unique(RefCountedPtr(this), cb), std::move(args), delegate_policy_name, @@ -96,7 +96,7 @@ class TestPickArgsLb : public ForwardingLoadBalancingPolicy { ~TestPickArgsLb() override = default; - const char* name() const override { return kTestPickArgsLbPolicyName; } + absl::string_view name() const override { return kTestPickArgsLbPolicyName; } private: class Picker : public SubchannelPicker { @@ -158,13 +158,13 @@ class TestPickArgsLb : public ForwardingLoadBalancingPolicy { class TestPickArgsLbConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { return kTestPickArgsLbPolicyName; } + absl::string_view name() const override { return kTestPickArgsLbPolicyName; } }; class TestPickArgsLbFactory : public LoadBalancingPolicyFactory { public: explicit TestPickArgsLbFactory(TestPickArgsCallback cb, - const char* delegate_policy_name) + absl::string_view delegate_policy_name) : cb_(std::move(cb)), delegate_policy_name_(delegate_policy_name) {} OrphanablePtr CreateLoadBalancingPolicy( @@ -173,16 +173,16 @@ class TestPickArgsLbFactory : public LoadBalancingPolicyFactory { delegate_policy_name_); } - const char* name() const override { return kTestPickArgsLbPolicyName; } + absl::string_view name() const override { return kTestPickArgsLbPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return MakeRefCounted(); } private: TestPickArgsCallback cb_; - const char* delegate_policy_name_; + std::string delegate_policy_name_; }; // @@ -208,7 +208,7 @@ class InterceptRecvTrailingMetadataLoadBalancingPolicy ~InterceptRecvTrailingMetadataLoadBalancingPolicy() override = default; - const char* name() const override { + absl::string_view name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } @@ -296,7 +296,7 @@ class InterceptRecvTrailingMetadataLoadBalancingPolicy class InterceptTrailingConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { + absl::string_view name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } }; @@ -312,12 +312,12 @@ class InterceptTrailingFactory : public LoadBalancingPolicyFactory { std::move(args), cb_); } - const char* name() const override { + absl::string_view name() const override { return kInterceptRecvTrailingMetadataLbPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return MakeRefCounted(); } @@ -344,7 +344,7 @@ class AddressTestLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { ~AddressTestLoadBalancingPolicy() override = default; - const char* name() const override { return kAddressTestLbPolicyName; } + absl::string_view name() const override { return kAddressTestLbPolicyName; } private: class Helper : public ChannelControlHelper { @@ -387,7 +387,7 @@ class AddressTestLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { class AddressTestConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { return kAddressTestLbPolicyName; } + absl::string_view name() const override { return kAddressTestLbPolicyName; } }; class AddressTestFactory : public LoadBalancingPolicyFactory { @@ -399,10 +399,10 @@ class AddressTestFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args), cb_); } - const char* name() const override { return kAddressTestLbPolicyName; } + absl::string_view name() const override { return kAddressTestLbPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return MakeRefCounted(); } @@ -421,7 +421,7 @@ class FixedAddressConfig : public LoadBalancingPolicy::Config { explicit FixedAddressConfig(std::string address) : address_(std::move(address)) {} - const char* name() const override { return kFixedAddressLbPolicyName; } + absl::string_view name() const override { return kFixedAddressLbPolicyName; } const std::string& address() const { return address_; } @@ -441,7 +441,7 @@ class FixedAddressLoadBalancingPolicy : public ForwardingLoadBalancingPolicy { ~FixedAddressLoadBalancingPolicy() override = default; - const char* name() const override { return kFixedAddressLbPolicyName; } + absl::string_view name() const override { return kFixedAddressLbPolicyName; } void UpdateLocked(UpdateArgs args) override { auto* config = static_cast(args.config.get()); @@ -508,17 +508,20 @@ class FixedAddressFactory : public LoadBalancingPolicyFactory { return MakeOrphanable(std::move(args)); } - const char* name() const override { return kFixedAddressLbPolicyName; } + absl::string_view name() const override { return kFixedAddressLbPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& json, grpc_error_handle* error) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& json) const override { std::vector error_list; std::string address; ParseJsonObjectField(json.object_value(), "address", &address, &error_list); if (!error_list.empty()) { - *error = GRPC_ERROR_CREATE_FROM_VECTOR( + grpc_error_handle error = GRPC_ERROR_CREATE_FROM_VECTOR( "errors parsing fixed_address_lb config", &error_list); - return nullptr; + absl::Status status = + absl::InvalidArgumentError(grpc_error_std_string(error)); + GRPC_ERROR_UNREF(error); + return status; } return MakeRefCounted(std::move(address)); } @@ -533,7 +536,7 @@ constexpr char kOobBackendMetricTestLbPolicyName[] = class OobBackendMetricTestConfig : public LoadBalancingPolicy::Config { public: - const char* name() const override { + absl::string_view name() const override { return kOobBackendMetricTestLbPolicyName; } }; @@ -553,7 +556,7 @@ class OobBackendMetricTestLoadBalancingPolicy ~OobBackendMetricTestLoadBalancingPolicy() override = default; - const char* name() const override { + absl::string_view name() const override { return kOobBackendMetricTestLbPolicyName; } @@ -629,12 +632,12 @@ class OobBackendMetricTestFactory : public LoadBalancingPolicyFactory { std::move(args), cb_); } - const char* name() const override { + absl::string_view name() const override { return kOobBackendMetricTestLbPolicyName; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /*json*/, grpc_error_handle* /*error*/) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return MakeRefCounted(); } @@ -644,8 +647,8 @@ class OobBackendMetricTestFactory : public LoadBalancingPolicyFactory { } // namespace -void RegisterTestPickArgsLoadBalancingPolicy(TestPickArgsCallback cb, - const char* delegate_policy_name) { +void RegisterTestPickArgsLoadBalancingPolicy( + TestPickArgsCallback cb, absl::string_view 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 d5815ef2921..fd4d6732be3 100644 --- a/test/core/util/test_lb_policies.h +++ b/test/core/util/test_lb_policies.h @@ -33,7 +33,8 @@ 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, const char* delegate_policy_name = "pick_first"); + TestPickArgsCallback cb, + absl::string_view 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 72e8bd41f8b..38ecedbe62c 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; } - const char* name() const override { return "test.CustomLb"; } + absl::string_view name() const override { return "test.CustomLb"; } - RefCountedPtr ParseLoadBalancingConfig( - const Json& /* json */, grpc_error_handle* /* error */) const override { + absl::StatusOr> + ParseLoadBalancingConfig(const Json& /*json*/) const override { return nullptr; } };