[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 <yashkt@google.com>

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",

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;
 };

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

src/core/ext/xds/xds_lb_policy_registry.cc

@@ -36,7 +36,6 @@
 
 #include <grpc/support/json.h>
 
-#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<WrrLocalityLbPolicyConfigFactory>());
-  if (ShufflePickFirstEnabled()) {
-    policy_config_factories_.emplace(
-        PickFirstLbPolicyConfigFactory::Type(),
-        std::make_unique<PickFirstLbPolicyConfigFactory>());
-  }
+  policy_config_factories_.emplace(
+      PickFirstLbPolicyConfigFactory::Type(),
+      std::make_unique<PickFirstLbPolicyConfigFactory>());
 }
 
 Json::Array XdsLbPolicyRegistry::ConvertXdsLbPolicyConfig(

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<absl::string_view, 6> 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<absl::string_view, 6> 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<absl::string_view, 6> 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<absl::string_view> address_order;
-    GetOrderAddressesArePicked(kAddresses, &address_order);
-    EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses));
-  }
-}
 }  // namespace
 }  // namespace testing
 }  // namespace grpc_core

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
 //

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_;