From 95c4f32b235cb403577d17464db6b2ba3cec592f Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 27 Mar 2024 23:18:17 +0000 Subject: [PATCH] [RegisteredMethod] Remove experiment (#36181) Closes #36181 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36181 from yashykt:RemoveRegisteredMethodLookupInTransport 3709bfa1cfec4ed95073a9788cf05a8eb14bd50e PiperOrigin-RevId: 619695276 --- bazel/experiments.bzl | 12 +------ src/core/lib/experiments/experiments.cc | 42 ++++------------------- src/core/lib/experiments/experiments.h | 11 ------ src/core/lib/experiments/experiments.yaml | 8 +---- src/core/lib/experiments/rollouts.yaml | 2 -- src/core/lib/surface/server.cc | 35 +++++-------------- test/cpp/end2end/BUILD | 1 - 7 files changed, 17 insertions(+), 94 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 80b0bd4b48b..bb08b829070 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -35,8 +35,7 @@ EXPERIMENT_ENABLES = { "promise_based_client_call": "event_engine_client,event_engine_listener,promise_based_client_call", "promise_based_server_call": "promise_based_server_call", "chaotic_good": "chaotic_good,event_engine_client,event_engine_listener,promise_based_client_call,promise_based_server_call", - "registered_method_lookup_in_transport": "registered_method_lookup_in_transport", - "promise_based_inproc_transport": "event_engine_client,event_engine_listener,promise_based_client_call,promise_based_inproc_transport,promise_based_server_call,registered_method_lookup_in_transport", + "promise_based_inproc_transport": "event_engine_client,event_engine_listener,promise_based_client_call,promise_based_inproc_transport,promise_based_server_call", "rstpit": "rstpit", "schedule_cancellation_over_write": "schedule_cancellation_over_write", "server_privacy": "server_privacy", @@ -96,9 +95,6 @@ EXPERIMENTS = { "event_engine_listener_test": [ "event_engine_listener", ], - "surface_registered_method_lookup": [ - "registered_method_lookup_in_transport", - ], }, }, "ios": { @@ -134,9 +130,6 @@ EXPERIMENTS = { ], }, "on": { - "surface_registered_method_lookup": [ - "registered_method_lookup_in_transport", - ], }, }, "posix": { @@ -200,9 +193,6 @@ EXPERIMENTS = { "resolver_component_tests_runner_invoker": [ "event_engine_dns", ], - "surface_registered_method_lookup": [ - "registered_method_lookup_in_transport", - ], "xds_end2end_test": [ "work_serializer_dispatch", ], diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 456fddeb398..808596cba0f 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -96,18 +96,12 @@ const char* const additional_constraints_chaotic_good = "{}"; const uint8_t required_experiments_chaotic_good[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; -const char* const description_registered_method_lookup_in_transport = - "Change registered method's lookup point to transport"; -const char* const additional_constraints_registered_method_lookup_in_transport = - "{}"; const char* const description_promise_based_inproc_transport = "Use promises for the in-process transport."; const char* const additional_constraints_promise_based_inproc_transport = "{}"; const uint8_t required_experiments_promise_based_inproc_transport[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), - static_cast(grpc_core::kExperimentIdPromiseBasedServerCall), - static_cast( - grpc_core::kExperimentIdRegisteredMethodLookupInTransport)}; + static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; const char* const description_rstpit = "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short " "duration"; @@ -201,14 +195,10 @@ const ExperimentMetadata g_experiment_metadata[] = { {"chaotic_good", description_chaotic_good, additional_constraints_chaotic_good, required_experiments_chaotic_good, 2, false, true}, - {"registered_method_lookup_in_transport", - description_registered_method_lookup_in_transport, - additional_constraints_registered_method_lookup_in_transport, nullptr, 0, - true, true}, {"promise_based_inproc_transport", description_promise_based_inproc_transport, additional_constraints_promise_based_inproc_transport, - required_experiments_promise_based_inproc_transport, 3, false, false}, + required_experiments_promise_based_inproc_transport, 2, false, false}, {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0, false, true}, {"schedule_cancellation_over_write", @@ -316,18 +306,12 @@ const char* const additional_constraints_chaotic_good = "{}"; const uint8_t required_experiments_chaotic_good[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; -const char* const description_registered_method_lookup_in_transport = - "Change registered method's lookup point to transport"; -const char* const additional_constraints_registered_method_lookup_in_transport = - "{}"; const char* const description_promise_based_inproc_transport = "Use promises for the in-process transport."; const char* const additional_constraints_promise_based_inproc_transport = "{}"; const uint8_t required_experiments_promise_based_inproc_transport[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), - static_cast(grpc_core::kExperimentIdPromiseBasedServerCall), - static_cast( - grpc_core::kExperimentIdRegisteredMethodLookupInTransport)}; + static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; const char* const description_rstpit = "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short " "duration"; @@ -421,14 +405,10 @@ const ExperimentMetadata g_experiment_metadata[] = { {"chaotic_good", description_chaotic_good, additional_constraints_chaotic_good, required_experiments_chaotic_good, 2, false, true}, - {"registered_method_lookup_in_transport", - description_registered_method_lookup_in_transport, - additional_constraints_registered_method_lookup_in_transport, nullptr, 0, - true, true}, {"promise_based_inproc_transport", description_promise_based_inproc_transport, additional_constraints_promise_based_inproc_transport, - required_experiments_promise_based_inproc_transport, 3, false, false}, + required_experiments_promise_based_inproc_transport, 2, false, false}, {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0, false, true}, {"schedule_cancellation_over_write", @@ -536,18 +516,12 @@ const char* const additional_constraints_chaotic_good = "{}"; const uint8_t required_experiments_chaotic_good[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; -const char* const description_registered_method_lookup_in_transport = - "Change registered method's lookup point to transport"; -const char* const additional_constraints_registered_method_lookup_in_transport = - "{}"; const char* const description_promise_based_inproc_transport = "Use promises for the in-process transport."; const char* const additional_constraints_promise_based_inproc_transport = "{}"; const uint8_t required_experiments_promise_based_inproc_transport[] = { static_cast(grpc_core::kExperimentIdPromiseBasedClientCall), - static_cast(grpc_core::kExperimentIdPromiseBasedServerCall), - static_cast( - grpc_core::kExperimentIdRegisteredMethodLookupInTransport)}; + static_cast(grpc_core::kExperimentIdPromiseBasedServerCall)}; const char* const description_rstpit = "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short " "duration"; @@ -641,14 +615,10 @@ const ExperimentMetadata g_experiment_metadata[] = { {"chaotic_good", description_chaotic_good, additional_constraints_chaotic_good, required_experiments_chaotic_good, 2, false, true}, - {"registered_method_lookup_in_transport", - description_registered_method_lookup_in_transport, - additional_constraints_registered_method_lookup_in_transport, nullptr, 0, - true, true}, {"promise_based_inproc_transport", description_promise_based_inproc_transport, additional_constraints_promise_based_inproc_transport, - required_experiments_promise_based_inproc_transport, 3, false, false}, + required_experiments_promise_based_inproc_transport, 2, false, false}, {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0, false, true}, {"schedule_cancellation_over_write", diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index a91dce2bdd4..172b3db8e9a 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -87,8 +87,6 @@ inline bool IsPendingQueueCapEnabled() { return true; } inline bool IsPromiseBasedClientCallEnabled() { return false; } inline bool IsPromiseBasedServerCallEnabled() { return false; } inline bool IsChaoticGoodEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT -inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } inline bool IsRstpitEnabled() { return false; } inline bool IsScheduleCancellationOverWriteEnabled() { return false; } @@ -135,8 +133,6 @@ inline bool IsPendingQueueCapEnabled() { return true; } inline bool IsPromiseBasedClientCallEnabled() { return false; } inline bool IsPromiseBasedServerCallEnabled() { return false; } inline bool IsChaoticGoodEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT -inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } inline bool IsRstpitEnabled() { return false; } inline bool IsScheduleCancellationOverWriteEnabled() { return false; } @@ -184,8 +180,6 @@ inline bool IsPendingQueueCapEnabled() { return true; } inline bool IsPromiseBasedClientCallEnabled() { return false; } inline bool IsPromiseBasedServerCallEnabled() { return false; } inline bool IsChaoticGoodEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT -inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } inline bool IsRstpitEnabled() { return false; } inline bool IsScheduleCancellationOverWriteEnabled() { return false; } @@ -222,7 +216,6 @@ enum ExperimentIds { kExperimentIdPromiseBasedClientCall, kExperimentIdPromiseBasedServerCall, kExperimentIdChaoticGood, - kExperimentIdRegisteredMethodLookupInTransport, kExperimentIdPromiseBasedInprocTransport, kExperimentIdRstpit, kExperimentIdScheduleCancellationOverWrite, @@ -309,10 +302,6 @@ inline bool IsPromiseBasedServerCallEnabled() { inline bool IsChaoticGoodEnabled() { return IsExperimentEnabled(kExperimentIdChaoticGood); } -#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT -inline bool IsRegisteredMethodLookupInTransportEnabled() { - return IsExperimentEnabled(kExperimentIdRegisteredMethodLookupInTransport); -} #define GRPC_EXPERIMENT_IS_INCLUDED_PROMISE_BASED_INPROC_TRANSPORT inline bool IsPromiseBasedInprocTransportEnabled() { return IsExperimentEnabled(kExperimentIdPromiseBasedInprocTransport); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index f786b53bc73..dd4bb55ca40 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -165,7 +165,7 @@ owner: ctiller@google.com test_tags: [] allow_in_fuzzing_config: false # experiment currently crashes if enabled - requires: [promise_based_client_call, promise_based_server_call, registered_method_lookup_in_transport] + requires: [promise_based_client_call, promise_based_server_call] - name: promise_based_server_call description: If set, use the new gRPC promise based call code when it's appropriate @@ -173,12 +173,6 @@ expiry: 2024/06/14 owner: ctiller@google.com test_tags: ["core_end2end_test", "cpp_end2end_test", "xds_end2end_test", "logging_test"] -- name: registered_method_lookup_in_transport - description: - Change registered method's lookup point to transport - expiry: 2024/03/31 - owner: yashkt@google.com - test_tags: ["surface_registered_method_lookup"] - name: rstpit description: On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short duration diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index 62933514aa5..ae0dd487d79 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -96,8 +96,6 @@ posix: false - name: promise_based_server_call default: false -- name: registered_method_lookup_in_transport - default: true - name: rstpit default: false - name: schedule_cancellation_over_write diff --git a/src/core/lib/surface/server.cc b/src/core/lib/surface/server.cc index 7cdd88dcdae..d9289e3b683 100644 --- a/src/core/lib/surface/server.cc +++ b/src/core/lib/surface/server.cc @@ -1330,13 +1330,10 @@ void Server::ChannelData::InitTransport(RefCountedPtr server, ++accept_stream_types; op->set_accept_stream = true; op->set_accept_stream_fn = AcceptStream; - if (IsRegisteredMethodLookupInTransportEnabled()) { - op->set_registered_method_matcher_fn = [](void* arg, - ClientMetadata* metadata) { - static_cast(arg)->SetRegisteredMethodOnMetadata( - *metadata); - }; - } + op->set_registered_method_matcher_fn = [](void* arg, + ClientMetadata* metadata) { + static_cast(arg)->SetRegisteredMethodOnMetadata(*metadata); + }; op->set_accept_stream_user_data = this; } if (transport->server_transport() != nullptr) { @@ -1527,15 +1524,9 @@ ArenaPromise Server::ChannelData::MakeCallPromise( } // Find request matcher. RequestMatcherInterface* matcher; - RegisteredMethod* rm = nullptr; - if (IsRegisteredMethodLookupInTransportEnabled()) { - rm = static_cast( - call_args.client_initial_metadata->get(GrpcRegisteredMethod()) - .value_or(nullptr)); - } else { - rm = chand->GetRegisteredMethod(host_ptr->as_string_view(), - path_ptr->as_string_view()); - } + RegisteredMethod* rm = static_cast( + call_args.client_initial_metadata->get(GrpcRegisteredMethod()) + .value_or(nullptr)); ArenaPromise>> maybe_read_first_message([] { return NextResult(); }); if (rm != nullptr) { @@ -1760,7 +1751,6 @@ void Server::CallData::KillZombie() { // If this changes, change MakeCallPromise too. void Server::CallData::StartNewRpc(grpc_call_element* elem) { - auto* chand = static_cast(elem->channel_data); if (server_->ShutdownCalled()) { state_.store(CallState::ZOMBIED, std::memory_order_relaxed); KillZombie(); @@ -1771,15 +1761,8 @@ void Server::CallData::StartNewRpc(grpc_call_element* elem) { grpc_server_register_method_payload_handling payload_handling = GRPC_SRM_PAYLOAD_NONE; if (path_.has_value() && host_.has_value()) { - RegisteredMethod* rm; - if (IsRegisteredMethodLookupInTransportEnabled()) { - rm = static_cast( - recv_initial_metadata_->get(GrpcRegisteredMethod()) - .value_or(nullptr)); - } else { - rm = chand->GetRegisteredMethod(host_->as_string_view(), - path_->as_string_view()); - } + RegisteredMethod* rm = static_cast( + recv_initial_metadata_->get(GrpcRegisteredMethod()).value_or(nullptr)); if (rm != nullptr) { matcher_ = rm->matcher.get(); payload_handling = rm->payload_handling; diff --git a/test/cpp/end2end/BUILD b/test/cpp/end2end/BUILD index 1a063cfe853..3e6c43ea53a 100644 --- a/test/cpp/end2end/BUILD +++ b/test/cpp/end2end/BUILD @@ -364,7 +364,6 @@ grpc_cc_test( tags = [ "cpp_end2end_test", "no_test_ios", - "surface_registered_method_lookup", ], deps = [ ":end2end_test_lib",