From ab024624da7bb918a2ee0c4bf4a6ab12bf464051 Mon Sep 17 00:00:00 2001 From: Mohan Li <67390330+mohanli-ml@users.noreply.github.com> Date: Thu, 17 Aug 2023 09:54:55 -0700 Subject: [PATCH] [pick_first] de-experiment pick first (#34054) De-experiment pick first since we have both affinity and randomness E2E test running successfully. --------- Co-authored-by: Yash Tibrewal --- src/core/BUILD | 2 -- .../lb_policy/pick_first/pick_first.cc | 19 ----------------- .../lb_policy/pick_first/pick_first.h | 5 ----- src/core/ext/xds/xds_lb_policy_registry.cc | 9 +++----- .../lb_policy/pick_first_test.cc | 21 ------------------- test/core/xds/xds_lb_policy_registry_test.cc | 13 ------------ .../xds/xds_pick_first_end2end_test.cc | 3 --- 7 files changed, 3 insertions(+), 69 deletions(-) diff --git a/src/core/BUILD b/src/core/BUILD index f192120a04a..9987426a321 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4731,7 +4731,6 @@ grpc_cc_library( language = "c++", deps = [ "channel_args", - "env", "grpc_lb_subchannel_list", "grpc_outlier_detection_header", "json", @@ -4740,7 +4739,6 @@ grpc_cc_library( "lb_policy", "lb_policy_factory", "subchannel_interface", - "validation_errors", "//:channel_arg_names", "//:config", "//:debug_location", 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 70d55be7a72..0feece30ca3 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 @@ -42,12 +42,9 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" -#include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/debug_location.h" -#include "src/core/lib/gprpp/env.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" -#include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/gprpp/work_serializer.h" #include "src/core/lib/json/json.h" #include "src/core/lib/json/json_args.h" @@ -62,15 +59,6 @@ namespace grpc_core { TraceFlag grpc_lb_pick_first_trace(false, "pick_first"); -// TODO(eostroukhov): Remove once this feature is no longer experimental. -bool ShufflePickFirstEnabled() { - auto value = GetEnv("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); - if (!value.has_value()) return false; - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value->c_str(), &parsed_value); - return parse_succeeded && parsed_value; -} - namespace { // @@ -93,13 +81,6 @@ class PickFirstConfig : public LoadBalancingPolicy::Config { return kJsonLoader; } - void JsonPostLoad(const Json& /* json */, const JsonArgs& /* args */, - ValidationErrors* /* errors */) { - if (!ShufflePickFirstEnabled()) { - shuffle_addresses_ = false; - } - } - private: bool shuffle_addresses_ = false; }; diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.h b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.h index fcd71d078a4..38b70e85f9a 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.h +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.h @@ -17,9 +17,4 @@ #ifndef GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_PICK_FIRST_PICK_FIRST_H #define GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_PICK_FIRST_PICK_FIRST_H -namespace grpc_core { -// TODO(eostroukhov): Remove once this feature is no longer experimental. -bool ShufflePickFirstEnabled(); -} // namespace grpc_core - #endif // GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_PICK_FIRST_PICK_FIRST_H diff --git a/src/core/ext/xds/xds_lb_policy_registry.cc b/src/core/ext/xds/xds_lb_policy_registry.cc index 9c97be9206c..a8cb6de128e 100644 --- a/src/core/ext/xds/xds_lb_policy_registry.cc +++ b/src/core/ext/xds/xds_lb_policy_registry.cc @@ -36,7 +36,6 @@ #include -#include "src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.h" #include "src/core/ext/xds/xds_common_types.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/gprpp/time.h" @@ -307,11 +306,9 @@ XdsLbPolicyRegistry::XdsLbPolicyRegistry() { policy_config_factories_.emplace( WrrLocalityLbPolicyConfigFactory::Type(), std::make_unique()); - if (ShufflePickFirstEnabled()) { - policy_config_factories_.emplace( - PickFirstLbPolicyConfigFactory::Type(), - std::make_unique()); - } + policy_config_factories_.emplace( + PickFirstLbPolicyConfigFactory::Type(), + std::make_unique()); } Json::Array XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig( diff --git a/test/core/client_channel/lb_policy/pick_first_test.cc b/test/core/client_channel/lb_policy/pick_first_test.cc index 43158dad12f..460d79f47a1 100644 --- a/test/core/client_channel/lb_policy/pick_first_test.cc +++ b/test/core/client_channel/lb_policy/pick_first_test.cc @@ -42,7 +42,6 @@ #include "src/core/lib/json/json.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "test/core/client_channel/lb_policy/lb_policy_test_lib.h" -#include "test/core/util/scoped_env_var.h" #include "test/core/util/test_config.h" namespace grpc_core { @@ -486,8 +485,6 @@ TEST_F(PickFirstTest, GoesIdleWhenConnectionFailsThenCanReconnect) { } TEST_F(PickFirstTest, WithShuffle) { - testing::ScopedExperimentalEnvVar env_var( - "GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; @@ -516,8 +513,6 @@ TEST_F(PickFirstTest, WithShuffle) { } TEST_F(PickFirstTest, ShufflingDisabled) { - testing::ScopedExperimentalEnvVar env_var( - "GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; @@ -531,22 +526,6 @@ TEST_F(PickFirstTest, ShufflingDisabled) { EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses)); } } - -// TODO(eugeneo): remove when the env var no longer necessary -TEST_F(PickFirstTest, ShufflingDisabledViaEnvVar) { - constexpr std::array kAddresses = { - "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444", "ipv4:127.0.0.1:445", - "ipv4:127.0.0.1:446", "ipv4:127.0.0.1:447", "ipv4:127.0.0.1:448"}; - constexpr static size_t kMaxAttempts = 5; - for (size_t attempt = 0; attempt < kMaxAttempts; ++attempt) { - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(true)), lb_policy_.get()); - EXPECT_TRUE(status.ok()) << status; - std::vector address_order; - GetOrderAddressesArePicked(kAddresses, &address_order); - EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses)); - } -} } // namespace } // namespace testing } // namespace grpc_core diff --git a/test/core/xds/xds_lb_policy_registry_test.cc b/test/core/xds/xds_lb_policy_registry_test.cc index b193ad5a442..53826317a33 100644 --- a/test/core/xds/xds_lb_policy_registry_test.cc +++ b/test/core/xds/xds_lb_policy_registry_test.cc @@ -53,7 +53,6 @@ #include "src/proto/grpc/testing/xds/v3/round_robin.pb.h" #include "src/proto/grpc/testing/xds/v3/typed_struct.pb.h" #include "src/proto/grpc/testing/xds/v3/wrr_locality.pb.h" -#include "test/core/util/scoped_env_var.h" #include "test/core/util/test_config.h" namespace grpc_core { @@ -450,7 +449,6 @@ TEST(WrrLocality, UnsupportedChildPolicyTypeSkipped) { // TEST(PickFirst, NoShuffle) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); PickFirst pick_first; @@ -463,7 +461,6 @@ TEST(PickFirst, NoShuffle) { } TEST(PickFirst, Shuffle) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); PickFirst pick_first; @@ -476,7 +473,6 @@ TEST(PickFirst, Shuffle) { } TEST(PickFirst, ShuffleOmitted) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); LoadBalancingPolicyProto policy; auto* lb_policy = policy.add_policies(); lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( @@ -486,15 +482,6 @@ TEST(PickFirst, ShuffleOmitted) { EXPECT_EQ(*result, "{\"pick_first\":{\"shuffleAddressList\":false}}"); } -TEST(PickFirst, EnvVarDisabled) { - LoadBalancingPolicyProto policy; - auto* lb_policy = policy.add_policies(); - lb_policy->mutable_typed_extension_config()->mutable_typed_config()->PackFrom( - PickFirst()); - auto result = ConvertXdsPolicy(policy); - ASSERT_FALSE(result.ok()); -} - // // CustomPolicy // diff --git a/test/cpp/end2end/xds/xds_pick_first_end2end_test.cc b/test/cpp/end2end/xds/xds_pick_first_end2end_test.cc index 4344c27a727..825642f7195 100644 --- a/test/cpp/end2end/xds/xds_pick_first_end2end_test.cc +++ b/test/cpp/end2end/xds/xds_pick_first_end2end_test.cc @@ -37,7 +37,6 @@ #include "src/core/lib/gprpp/env.h" #include "src/proto/grpc/testing/xds/v3/cluster.grpc.pb.h" #include "src/proto/grpc/testing/xds/v3/pick_first.pb.h" -#include "test/core/util/scoped_env_var.h" #include "test/core/util/test_config.h" #include "test/cpp/end2end/connection_attempt_injector.h" #include "test/cpp/end2end/xds/xds_end2end_test_lib.h" @@ -80,8 +79,6 @@ INSTANTIATE_TEST_SUITE_P(XdsTest, PickFirstTest, ::testing::Values(XdsTestType()), &XdsTestType::Name); TEST_P(PickFirstTest, PickFirstConfigurationIsPropagated) { - grpc_core::testing::ScopedExperimentalEnvVar env_var( - "GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG"); CreateAndStartBackends(6); // Change cluster to use pick_first with shuffle option. auto cluster = default_cluster_;