From d59c8eb0f5f38afc3114029517e9575206ffe710 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Tue, 20 Jun 2023 11:25:01 -0700 Subject: [PATCH] Revert "[lb pick_first] Enable random shuffling of address list (#33254)" (#33496) Original PR: 33254 This reverts commit 7e14a322a24b01e1bef77dca94156233494c734a. --- CMakeLists.txt | 10 +- build_autogenerated.yaml | 3 - gRPC-C++.podspec | 1 - gRPC-Core.podspec | 1 - grpc.gyp | 2 - src/core/BUILD | 6 - .../lb_policy/pick_first/pick_first.cc | 58 +------ test/core/client_channel/lb_policy/BUILD | 1 - .../lb_policy/lb_policy_test_lib.h | 2 +- .../lb_policy/pick_first_test.cc | 164 +----------------- .../lb_policy/round_robin_test.cc | 16 +- .../lb_policy/xds_override_host_test.cc | 5 +- test/core/util/test_lb_policies.cc | 8 - 13 files changed, 26 insertions(+), 251 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e89aac8edf0..5ccedc14a34 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2478,7 +2478,6 @@ target_link_libraries(grpc ${_gRPC_RE2_LIBRARIES} ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} - absl::algorithm_container absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -3123,7 +3122,6 @@ target_link_libraries(grpc_unsecure ${_gRPC_RE2_LIBRARIES} ${_gRPC_UPB_LIBRARIES} ${_gRPC_ALLTARGETS_LIBRARIES} - absl::algorithm_container absl::cleanup absl::flat_hash_map absl::flat_hash_set @@ -30871,7 +30869,7 @@ generate_pkgconfig( "gRPC" "high performance general RPC framework" "${gRPC_CORE_VERSION}" - "gpr absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "openssl re2 libcares zlib" "-lgrpc" "-laddress_sorting -lupb" @@ -30882,7 +30880,7 @@ generate_pkgconfig( "gRPC unsecure" "high performance general RPC framework without SSL" "${gRPC_CORE_VERSION}" - "gpr absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "gpr absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "re2 libcares zlib" "-lgrpc_unsecure" "-laddress_sorting -lupb" @@ -30893,7 +30891,7 @@ generate_pkgconfig( "gRPC++" "C++ wrapper for gRPC" "${gRPC_CPP_VERSION}" - "grpc absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "" "-lgrpc++" "" @@ -30904,7 +30902,7 @@ generate_pkgconfig( "gRPC++ unsecure" "C++ wrapper for gRPC without SSL" "${gRPC_CPP_VERSION}" - "grpc_unsecure absl_algorithm_container absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" + "grpc_unsecure absl_any_invocable absl_base absl_bind_front absl_cleanup absl_cord absl_core_headers absl_flags absl_flags_marshalling absl_flat_hash_map absl_flat_hash_set absl_function_ref absl_hash absl_inlined_vector absl_memory absl_optional absl_random_random absl_span absl_status absl_statusor absl_str_format absl_strings absl_synchronization absl_time absl_type_traits absl_utility absl_variant" "" "-lgrpc++_unsecure" "" diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 9b9f77622ff..1a144d20a9f 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1781,7 +1781,6 @@ libs: - src/core/tsi/transport_security.cc - src/core/tsi/transport_security_grpc.cc deps: - - absl/algorithm:container - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -2716,7 +2715,6 @@ libs: - src/core/tsi/transport_security.cc - src/core/tsi/transport_security_grpc.cc deps: - - absl/algorithm:container - absl/cleanup:cleanup - absl/container:flat_hash_map - absl/container:flat_hash_set @@ -11254,7 +11252,6 @@ targets: headers: - test/core/client_channel/lb_policy/lb_policy_test_lib.h - test/core/event_engine/mock_event_engine.h - - test/core/util/scoped_env_var.h src: - test/core/client_channel/lb_policy/pick_first_test.cc deps: diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 918cacc9687..4f8d3a4a6e9 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -216,7 +216,6 @@ Pod::Spec.new do |s| ss.dependency "#{s.name}/Interface", version ss.dependency 'gRPC-Core', version abseil_version = '1.20230125.3' - ss.dependency 'abseil/algorithm/container', abseil_version ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 760ddacbdd4..dc417abcc66 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -183,7 +183,6 @@ Pod::Spec.new do |s| ss.libraries = 'z' ss.dependency "#{s.name}/Interface", version ss.dependency 'BoringSSL-GRPC', '0.0.29' - ss.dependency 'abseil/algorithm/container', abseil_version ss.dependency 'abseil/base/base', abseil_version ss.dependency 'abseil/base/core_headers', abseil_version ss.dependency 'abseil/cleanup/cleanup', abseil_version diff --git a/grpc.gyp b/grpc.gyp index d5245c52b39..81657590cc5 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -249,7 +249,6 @@ 'target_name': 'grpc', 'type': 'static_library', 'dependencies': [ - 'absl/algorithm:container', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', @@ -1080,7 +1079,6 @@ 'target_name': 'grpc_unsecure', 'type': 'static_library', 'dependencies': [ - 'absl/algorithm:container', 'absl/cleanup:cleanup', 'absl/container:flat_hash_map', 'absl/container:flat_hash_set', diff --git a/src/core/BUILD b/src/core/BUILD index 56bc93eba7f..21eff2788f2 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -4624,8 +4624,6 @@ grpc_cc_library( "ext/filters/client_channel/lb_policy/pick_first/pick_first.cc", ], external_deps = [ - "absl/algorithm:container", - "absl/random", "absl/status", "absl/status:statusor", "absl/strings", @@ -4634,16 +4632,12 @@ grpc_cc_library( language = "c++", deps = [ "channel_args", - "env", "grpc_lb_subchannel_list", "grpc_outlier_detection_header", "json", - "json_args", - "json_object_loader", "lb_policy", "lb_policy_factory", "subchannel_interface", - "validation_errors", "//:config", "//:debug_location", "//:gpr", 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 8cc2b158e73..d3c3766731d 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 @@ -25,8 +25,6 @@ #include #include -#include "absl/algorithm/container.h" -#include "absl/random/random.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" @@ -42,16 +40,11 @@ #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" -#include "src/core/lib/json/json_object_loader.h" #include "src/core/lib/load_balancing/lb_policy.h" #include "src/core/lib/load_balancing/lb_policy_factory.h" #include "src/core/lib/load_balancing/subchannel_interface.h" @@ -70,40 +63,6 @@ namespace { constexpr absl::string_view kPickFirst = "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; -} - -class PickFirstConfig : public LoadBalancingPolicy::Config { - public: - absl::string_view name() const override { return kPickFirst; } - bool shuffle_addresses() const { return shuffle_addresses_; } - - static const JsonLoaderInterface* JsonLoader(const JsonArgs&) { - static const auto kJsonLoader = - JsonObjectLoader() - .OptionalField("shuffleAddressList", - &PickFirstConfig::shuffle_addresses_) - .Finish(); - return kJsonLoader; - } - - void JsonPostLoad(const Json& /* json */, const JsonArgs& /* args */, - ValidationErrors* /* errors */) { - if (!ShufflePickFirstEnabled()) { - shuffle_addresses_ = false; - } - } - - private: - bool shuffle_addresses_ = false; -}; - class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(Args args); @@ -210,8 +169,6 @@ class PickFirst : public LoadBalancingPolicy { bool idle_ = false; // Are we shut down? bool shutdown_ = false; - // Random bit generator used for shuffling addresses if configured - absl::BitGen bit_gen_; }; PickFirst::PickFirst(Args args) : LoadBalancingPolicy(std::move(args)) { @@ -317,11 +274,6 @@ absl::Status PickFirst::UpdateLocked(UpdateArgs args) { status = args.addresses.status(); } else if (args.addresses->empty()) { status = absl::UnavailableError("address list must not be empty"); - } else { - auto config = static_cast(args.config.get()); - if (config->shuffle_addresses()) { - absl::c_shuffle(*args.addresses, bit_gen_); - } } // TODO(roth): This is a hack to disable outlier_detection when used // with pick_first, for the reasons described in @@ -561,6 +513,11 @@ void PickFirst::PickFirstSubchannelData::ProcessUnselectedReadyLocked() { } } +class PickFirstConfig : public LoadBalancingPolicy::Config { + public: + absl::string_view name() const override { return kPickFirst; } +}; + // // factory // @@ -575,9 +532,8 @@ class PickFirstFactory : public LoadBalancingPolicyFactory { absl::string_view name() const override { return kPickFirst; } absl::StatusOr> - ParseLoadBalancingConfig(const Json& json) const override { - return LoadFromJson>( - json, JsonArgs(), "errors validating pick_first LB policy config"); + ParseLoadBalancingConfig(const Json& /*json*/) const override { + return MakeRefCounted(); } }; diff --git a/test/core/client_channel/lb_policy/BUILD b/test/core/client_channel/lb_policy/BUILD index cb69061d29f..5ed380f4579 100644 --- a/test/core/client_channel/lb_policy/BUILD +++ b/test/core/client_channel/lb_policy/BUILD @@ -50,7 +50,6 @@ grpc_cc_test( "//src/core:channel_args", "//src/core:grpc_lb_policy_pick_first", "//test/core/util:grpc_test_util", - "//test/core/util:scoped_env_var", ], ) diff --git a/test/core/client_channel/lb_policy/lb_policy_test_lib.h b/test/core/client_channel/lb_policy/lb_policy_test_lib.h index 70c4b1bbe34..95af80f806d 100644 --- a/test/core/client_channel/lb_policy/lb_policy_test_lib.h +++ b/test/core/client_channel/lb_policy/lb_policy_test_lib.h @@ -598,7 +598,7 @@ class LoadBalancingPolicyTest : public ::testing::Test { // Constructs an update containing a list of addresses. LoadBalancingPolicy::UpdateArgs BuildUpdate( absl::Span addresses, - RefCountedPtr config) { + RefCountedPtr config = nullptr) { LoadBalancingPolicy::UpdateArgs update; update.addresses.emplace(); for (const absl::string_view& address : addresses) { 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 46d833d99c2..98025614048 100644 --- a/test/core/client_channel/lb_policy/pick_first_test.cc +++ b/test/core/client_channel/lb_policy/pick_first_test.cc @@ -16,32 +16,19 @@ #include -#include #include -#include -#include -#include -#include #include "absl/status/status.h" #include "absl/strings/string_view.h" -#include "absl/types/optional.h" -#include "absl/types/span.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" #include -#include #include "src/core/lib/channel/channel_args.h" -#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/gprpp/work_serializer.h" -#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 { @@ -52,80 +39,6 @@ class PickFirstTest : public LoadBalancingPolicyTest { protected: PickFirstTest() : lb_policy_(MakeLbPolicy("pick_first")) {} - static RefCountedPtr MakePickFirstConfig( - absl::optional shuffle_address_list = absl::nullopt) { - return MakeConfig(Json::FromArray({Json::FromObject( - {{"pick_first", - shuffle_address_list.has_value() - ? Json::FromObject({{"shuffleAddressList", - Json::FromBool(*shuffle_address_list)}}) - : Json::FromObject({})}})})); - } - - // Gets order the addresses are being picked. Return type is void so - // assertions can be used - void GetOrderAddressesArePicked( - absl::Span addresses, - std::vector* out_address_order) { - work_serializer_->Run([&]() { lb_policy_->ExitIdleLocked(); }, - DEBUG_LOCATION); - out_address_order->clear(); - // Construct a map of subchannel to address. - // We will remove entries as each subchannel starts to connect. - std::map subchannels; - for (auto address : addresses) { - auto* subchannel = FindSubchannel( - address, ChannelArgs().Set(GRPC_ARG_INHIBIT_HEALTH_CHECKING, true)); - ASSERT_NE(subchannel, nullptr); - subchannels.emplace(subchannel, address); - } - // Now process each subchannel in the order in which pick_first tries it. - while (!subchannels.empty()) { - // Find the subchannel that is being attempted. - SubchannelState* subchannel = nullptr; - for (const auto& p : subchannels) { - if (p.first->ConnectionRequested()) { - out_address_order->push_back(p.second); - subchannel = p.first; - break; - } - } - ASSERT_NE(subchannel, nullptr); - // The subchannel reports CONNECTING. - subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); - // If this is the first subchannel being attempted, expect a CONNECTING - // update. - if (subchannels.size() == addresses.size()) { - ExpectConnectingUpdate(); - } - if (subchannels.size() > 1) { - // Not the last subchannel in the list. Connection attempt should fail. - subchannel->SetConnectivityState( - GRPC_CHANNEL_TRANSIENT_FAILURE, - absl::UnavailableError("failed to connect")); - subchannel->SetConnectivityState(GRPC_CHANNEL_IDLE); - } else { - // Last subchannel in the list. Connection attempt should succeed. - subchannel->SetConnectivityState(GRPC_CHANNEL_READY); - auto picker = WaitForConnected(); - ASSERT_NE(picker, nullptr); - EXPECT_EQ(ExpectPickComplete(picker.get()), out_address_order->back()); - // Then it should become disconnected. - subchannel->SetConnectivityState(GRPC_CHANNEL_IDLE); - ExpectReresolutionRequest(); - // We would normally call ExpectStateAndQueueingPicker() here instead of - // just ExpectState(). However, calling the picker would also trigger - // exiting IDLE, which we don't want here, because if the test is going - // to send an address list update and call GetOrderAddressesArePicked() - // again, we don't want to trigger a connection attempt on any - // subchannel until after that next address list update is processed. - ExpectState(GRPC_CHANNEL_IDLE); - } - // Remove the subchannel from the map. - subchannels.erase(subchannel); - } - } - OrphanablePtr lb_policy_; }; @@ -133,8 +46,7 @@ TEST_F(PickFirstTest, FirstAddressWorks) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); + absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -169,8 +81,7 @@ TEST_F(PickFirstTest, FirstAddressFails) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); + absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -183,8 +94,7 @@ TEST_F(PickFirstTest, FirstAddressFails) { // When the LB policy receives the first subchannel's initial connectivity // state notification (IDLE), it will request a connection. EXPECT_TRUE(subchannel->ConnectionRequested()); - // This causes the subchannel to start to connect, so it reports - // CONNECTING. + // This causes the subchannel to start to connect, so it reports CONNECTING. subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING); // LB policy should have reported CONNECTING state. ExpectConnectingUpdate(); @@ -195,8 +105,7 @@ TEST_F(PickFirstTest, FirstAddressFails) { absl::UnavailableError("failed to connect")); // The LB policy will start a connection attempt on the second subchannel. EXPECT_TRUE(subchannel2->ConnectionRequested()); - // This causes the subchannel to start to connect, so it reports - // CONNECTING. + // This causes the subchannel to start to connect, so it reports CONNECTING. subchannel2->SetConnectivityState(GRPC_CHANNEL_CONNECTING); // The connection attempt succeeds. subchannel2->SetConnectivityState(GRPC_CHANNEL_READY); @@ -214,8 +123,7 @@ TEST_F(PickFirstTest, GoesIdleWhenConnectionFailsThenCanReconnect) { // Send an update containing two addresses. constexpr std::array kAddresses = { "ipv4:127.0.0.1:443", "ipv4:127.0.0.1:444"}; - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); + absl::Status status = ApplyUpdate(BuildUpdate(kAddresses), lb_policy_.get()); EXPECT_TRUE(status.ok()) << status; // LB policy should have created a subchannel for both addresses with // the GRPC_ARG_INHIBIT_HEALTH_CHECKING channel arg. @@ -269,68 +177,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"}; - // 6 addresses have 6! = 720 permutations or roughly 0.14% chance that - // the shuffle returns same permutation. We allow for several tries to - // prevent flake test. - constexpr size_t kMaxTries = 10; - std::vector addresses_after_update; - bool shuffled = false; - for (size_t i = 0; i < kMaxTries; ++i) { - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(true)), lb_policy_.get()); - EXPECT_TRUE(status.ok()) << status; - GetOrderAddressesArePicked(kAddresses, &addresses_after_update); - if (absl::MakeConstSpan(addresses_after_update) != - absl::MakeConstSpan(kAddresses)) { - shuffled = true; - break; - } - } - ASSERT_TRUE(shuffled); - // Address order should be stable between updates - std::vector addresses_on_another_try; - GetOrderAddressesArePicked(kAddresses, &addresses_on_another_try); - EXPECT_EQ(addresses_on_another_try, addresses_after_update); -} - -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"}; - constexpr static size_t kMaxAttempts = 5; - for (size_t attempt = 0; attempt < kMaxAttempts; ++attempt) { - absl::Status status = ApplyUpdate( - BuildUpdate(kAddresses, MakePickFirstConfig(false)), lb_policy_.get()); - EXPECT_TRUE(status.ok()) << status; - std::vector address_order; - GetOrderAddressesArePicked(kAddresses, &address_order); - 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/client_channel/lb_policy/round_robin_test.cc b/test/core/client_channel/lb_policy/round_robin_test.cc index ef82ceadebf..72d720722be 100644 --- a/test/core/client_channel/lb_policy/round_robin_test.cc +++ b/test/core/client_channel/lb_policy/round_robin_test.cc @@ -40,7 +40,7 @@ class RoundRobinTest : public LoadBalancingPolicyTest { RoundRobinTest() : lb_policy_(MakeLbPolicy("round_robin")) {} void ExpectStartup(absl::Span addresses) { - EXPECT_EQ(ApplyUpdate(BuildUpdate(addresses, nullptr), lb_policy_.get()), + EXPECT_EQ(ApplyUpdate(BuildUpdate(addresses), lb_policy_.get()), absl::OkStatus()); // Expect the initial CONNECTNG update with a picker that queues. ExpectConnectingUpdate(); @@ -90,16 +90,14 @@ TEST_F(RoundRobinTest, AddressUpdates) { "ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442", "ipv4:127.0.0.1:443"}; ExpectStartup(kAddresses); // Send update to remove address 2. - EXPECT_EQ( - ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).first(2), nullptr), - lb_policy_.get()), - absl::OkStatus()); + EXPECT_EQ(ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).first(2)), + lb_policy_.get()), + absl::OkStatus()); WaitForRoundRobinListChange(kAddresses, absl::MakeSpan(kAddresses).first(2)); // Send update to remove address 0 and re-add address 2. - EXPECT_EQ( - ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).last(2), nullptr), - lb_policy_.get()), - absl::OkStatus()); + EXPECT_EQ(ApplyUpdate(BuildUpdate(absl::MakeSpan(kAddresses).last(2)), + lb_policy_.get()), + absl::OkStatus()); WaitForRoundRobinListChange(absl::MakeSpan(kAddresses).first(2), absl::MakeSpan(kAddresses).last(2)); } diff --git a/test/core/client_channel/lb_policy/xds_override_host_test.cc b/test/core/client_channel/lb_policy/xds_override_host_test.cc index 541682aff6d..c25d957e4b3 100644 --- a/test/core/client_channel/lb_policy/xds_override_host_test.cc +++ b/test/core/client_channel/lb_policy/xds_override_host_test.cc @@ -119,9 +119,8 @@ TEST_F(XdsOverrideHostTest, DelegatesToChild) { TEST_F(XdsOverrideHostTest, NoConfigReportsError) { EXPECT_EQ( - ApplyUpdate( - BuildUpdate({"ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442"}, nullptr), - policy_.get()), + ApplyUpdate(BuildUpdate({"ipv4:127.0.0.1:441", "ipv4:127.0.0.1:442"}), + policy_.get()), absl::InvalidArgumentError("Missing policy config")); } diff --git a/test/core/util/test_lb_policies.cc b/test/core/util/test_lb_policies.cc index 543cd4f5ddc..e24dde223d5 100644 --- a/test/core/util/test_lb_policies.cc +++ b/test/core/util/test_lb_policies.cc @@ -27,7 +27,6 @@ #include "absl/types/variant.h" #include -#include #include #include "src/core/ext/filters/client_channel/lb_policy/oob_backend_metric.h" @@ -79,13 +78,6 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy { ~ForwardingLoadBalancingPolicy() override = default; absl::Status UpdateLocked(UpdateArgs args) override { - // Use correct config for the delegate load balancing policy - auto config = - CoreConfiguration::Get().lb_policy_registry().ParseLoadBalancingConfig( - Json::FromArray({Json::FromObject( - {{std::string(delegate_->name()), Json::FromObject({})}})})); - GPR_ASSERT(config.ok()); - args.config = *config; return delegate_->UpdateLocked(std::move(args)); }