From 4fd48b1ba9b918a3172219a3c3cc035625953800 Mon Sep 17 00:00:00 2001 From: Vignesh Babu Date: Tue, 13 Aug 2024 10:26:19 -0700 Subject: [PATCH] Remove call_status_override_on_cancellation, peer_state_based_framing experiments. The peer_state_based_framing experiment is not used. The other experiment has been rolled out to 100% in prod for a while now. The expiry date of a few other experiments are updated. PiperOrigin-RevId: 662565880 --- bazel/experiments.bzl | 5 --- src/core/lib/experiments/experiments.cc | 48 ----------------------- src/core/lib/experiments/experiments.h | 19 --------- src/core/lib/experiments/experiments.yaml | 25 +++--------- src/core/lib/experiments/rollouts.yaml | 4 -- src/core/lib/surface/filter_stack_call.cc | 16 ++++---- 6 files changed, 12 insertions(+), 105 deletions(-) diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index 64a197963a15d..cd611fe7b0d17 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -17,7 +17,6 @@ """Dictionary of tags to experiments so we know when to test different experiments.""" EXPERIMENT_ENABLES = { - "call_status_override_on_cancellation": "call_status_override_on_cancellation", "call_tracer_in_transport": "call_tracer_in_transport", "canary_client_privacy": "canary_client_privacy", "client_privacy": "client_privacy", @@ -28,7 +27,6 @@ EXPERIMENT_ENABLES = { "max_pings_wo_data_throttle": "max_pings_wo_data_throttle", "monitoring_experiment": "monitoring_experiment", "multiping": "multiping", - "peer_state_based_framing": "peer_state_based_framing", "pick_first_new": "pick_first_new", "promise_based_inproc_transport": "promise_based_inproc_transport", "schedule_cancellation_over_write": "schedule_cancellation_over_write", @@ -58,7 +56,6 @@ EXPERIMENTS = { ], "flow_control_test": [ "multiping", - "peer_state_based_framing", "tcp_frame_size_tuning", "tcp_rcv_lowat", ], @@ -105,7 +102,6 @@ EXPERIMENTS = { ], "flow_control_test": [ "multiping", - "peer_state_based_framing", "tcp_frame_size_tuning", "tcp_rcv_lowat", ], @@ -146,7 +142,6 @@ EXPERIMENTS = { ], "flow_control_test": [ "multiping", - "peer_state_based_framing", "tcp_frame_size_tuning", "tcp_rcv_lowat", ], diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 5ce7bfb84f4d9..a9cabf1cb2927 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -22,11 +22,6 @@ #if defined(GRPC_CFSTREAM) namespace { -const char* const description_call_status_override_on_cancellation = - "Avoid overriding call status of successfully finished calls if it races " - "with cancellation."; -const char* const additional_constraints_call_status_override_on_cancellation = - "{}"; const char* const description_call_tracer_in_transport = "Transport directly passes byte counts to CallTracer."; const char* const additional_constraints_call_tracer_in_transport = "{}"; @@ -58,11 +53,6 @@ const char* const additional_constraints_monitoring_experiment = "{}"; const char* const description_multiping = "Allow more than one ping to be in flight at a time by default."; const char* const additional_constraints_multiping = "{}"; -const char* const description_peer_state_based_framing = - "If set, the max sizes of frames sent to lower layers is controlled based " - "on the peer's memory pressure which is reflected in its max http2 frame " - "size."; -const char* const additional_constraints_peer_state_based_framing = "{}"; const char* const description_pick_first_new = "New pick_first impl with memory reduction."; const char* const additional_constraints_pick_first_new = "{}"; @@ -105,10 +95,6 @@ const char* const additional_constraints_work_serializer_dispatch = "{}"; namespace grpc_core { const ExperimentMetadata g_experiment_metadata[] = { - {"call_status_override_on_cancellation", - description_call_status_override_on_cancellation, - additional_constraints_call_status_override_on_cancellation, nullptr, 0, - true, true}, {"call_tracer_in_transport", description_call_tracer_in_transport, additional_constraints_call_tracer_in_transport, nullptr, 0, true, true}, {"canary_client_privacy", description_canary_client_privacy, @@ -130,8 +116,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, nullptr, 0, false, true}, - {"peer_state_based_framing", description_peer_state_based_framing, - additional_constraints_peer_state_based_framing, nullptr, 0, false, true}, {"pick_first_new", description_pick_first_new, additional_constraints_pick_first_new, nullptr, 0, true, true}, {"promise_based_inproc_transport", @@ -166,11 +150,6 @@ const ExperimentMetadata g_experiment_metadata[] = { #elif defined(GPR_WINDOWS) namespace { -const char* const description_call_status_override_on_cancellation = - "Avoid overriding call status of successfully finished calls if it races " - "with cancellation."; -const char* const additional_constraints_call_status_override_on_cancellation = - "{}"; const char* const description_call_tracer_in_transport = "Transport directly passes byte counts to CallTracer."; const char* const additional_constraints_call_tracer_in_transport = "{}"; @@ -202,11 +181,6 @@ const char* const additional_constraints_monitoring_experiment = "{}"; const char* const description_multiping = "Allow more than one ping to be in flight at a time by default."; const char* const additional_constraints_multiping = "{}"; -const char* const description_peer_state_based_framing = - "If set, the max sizes of frames sent to lower layers is controlled based " - "on the peer's memory pressure which is reflected in its max http2 frame " - "size."; -const char* const additional_constraints_peer_state_based_framing = "{}"; const char* const description_pick_first_new = "New pick_first impl with memory reduction."; const char* const additional_constraints_pick_first_new = "{}"; @@ -249,10 +223,6 @@ const char* const additional_constraints_work_serializer_dispatch = "{}"; namespace grpc_core { const ExperimentMetadata g_experiment_metadata[] = { - {"call_status_override_on_cancellation", - description_call_status_override_on_cancellation, - additional_constraints_call_status_override_on_cancellation, nullptr, 0, - true, true}, {"call_tracer_in_transport", description_call_tracer_in_transport, additional_constraints_call_tracer_in_transport, nullptr, 0, true, true}, {"canary_client_privacy", description_canary_client_privacy, @@ -274,8 +244,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, nullptr, 0, false, true}, - {"peer_state_based_framing", description_peer_state_based_framing, - additional_constraints_peer_state_based_framing, nullptr, 0, false, true}, {"pick_first_new", description_pick_first_new, additional_constraints_pick_first_new, nullptr, 0, true, true}, {"promise_based_inproc_transport", @@ -310,11 +278,6 @@ const ExperimentMetadata g_experiment_metadata[] = { #else namespace { -const char* const description_call_status_override_on_cancellation = - "Avoid overriding call status of successfully finished calls if it races " - "with cancellation."; -const char* const additional_constraints_call_status_override_on_cancellation = - "{}"; const char* const description_call_tracer_in_transport = "Transport directly passes byte counts to CallTracer."; const char* const additional_constraints_call_tracer_in_transport = "{}"; @@ -346,11 +309,6 @@ const char* const additional_constraints_monitoring_experiment = "{}"; const char* const description_multiping = "Allow more than one ping to be in flight at a time by default."; const char* const additional_constraints_multiping = "{}"; -const char* const description_peer_state_based_framing = - "If set, the max sizes of frames sent to lower layers is controlled based " - "on the peer's memory pressure which is reflected in its max http2 frame " - "size."; -const char* const additional_constraints_peer_state_based_framing = "{}"; const char* const description_pick_first_new = "New pick_first impl with memory reduction."; const char* const additional_constraints_pick_first_new = "{}"; @@ -393,10 +351,6 @@ const char* const additional_constraints_work_serializer_dispatch = "{}"; namespace grpc_core { const ExperimentMetadata g_experiment_metadata[] = { - {"call_status_override_on_cancellation", - description_call_status_override_on_cancellation, - additional_constraints_call_status_override_on_cancellation, nullptr, 0, - true, true}, {"call_tracer_in_transport", description_call_tracer_in_transport, additional_constraints_call_tracer_in_transport, nullptr, 0, true, true}, {"canary_client_privacy", description_canary_client_privacy, @@ -418,8 +372,6 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_monitoring_experiment, nullptr, 0, true, true}, {"multiping", description_multiping, additional_constraints_multiping, nullptr, 0, false, true}, - {"peer_state_based_framing", description_peer_state_based_framing, - additional_constraints_peer_state_based_framing, nullptr, 0, false, true}, {"pick_first_new", description_pick_first_new, additional_constraints_pick_first_new, nullptr, 0, true, true}, {"promise_based_inproc_transport", diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 89f6b6010ae90..fa3ddaa812741 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -57,8 +57,6 @@ namespace grpc_core { #ifdef GRPC_EXPERIMENTS_ARE_FINAL #if defined(GRPC_CFSTREAM) -#define GRPC_EXPERIMENT_IS_INCLUDED_CALL_STATUS_OVERRIDE_ON_CANCELLATION -inline bool IsCallStatusOverrideOnCancellationEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_CALL_TRACER_IN_TRANSPORT inline bool IsCallTracerInTransportEnabled() { return true; } inline bool IsCanaryClientPrivacyEnabled() { return false; } @@ -71,7 +69,6 @@ inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } -inline bool IsPeerStateBasedFramingEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_PICK_FIRST_NEW inline bool IsPickFirstNewEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } @@ -87,8 +84,6 @@ inline bool IsWorkSerializerClearsTimeCacheEnabled() { return true; } inline bool IsWorkSerializerDispatchEnabled() { return false; } #elif defined(GPR_WINDOWS) -#define GRPC_EXPERIMENT_IS_INCLUDED_CALL_STATUS_OVERRIDE_ON_CANCELLATION -inline bool IsCallStatusOverrideOnCancellationEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_CALL_TRACER_IN_TRANSPORT inline bool IsCallTracerInTransportEnabled() { return true; } inline bool IsCanaryClientPrivacyEnabled() { return false; } @@ -104,7 +99,6 @@ inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } -inline bool IsPeerStateBasedFramingEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_PICK_FIRST_NEW inline bool IsPickFirstNewEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } @@ -120,8 +114,6 @@ inline bool IsWorkSerializerClearsTimeCacheEnabled() { return true; } inline bool IsWorkSerializerDispatchEnabled() { return false; } #else -#define GRPC_EXPERIMENT_IS_INCLUDED_CALL_STATUS_OVERRIDE_ON_CANCELLATION -inline bool IsCallStatusOverrideOnCancellationEnabled() { return true; } #define GRPC_EXPERIMENT_IS_INCLUDED_CALL_TRACER_IN_TRANSPORT inline bool IsCallTracerInTransportEnabled() { return true; } inline bool IsCanaryClientPrivacyEnabled() { return false; } @@ -136,7 +128,6 @@ inline bool IsMaxPingsWoDataThrottleEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT inline bool IsMonitoringExperimentEnabled() { return true; } inline bool IsMultipingEnabled() { return false; } -inline bool IsPeerStateBasedFramingEnabled() { return false; } #define GRPC_EXPERIMENT_IS_INCLUDED_PICK_FIRST_NEW inline bool IsPickFirstNewEnabled() { return true; } inline bool IsPromiseBasedInprocTransportEnabled() { return false; } @@ -154,7 +145,6 @@ inline bool IsWorkSerializerDispatchEnabled() { return false; } #else enum ExperimentIds { - kExperimentIdCallStatusOverrideOnCancellation, kExperimentIdCallTracerInTransport, kExperimentIdCanaryClientPrivacy, kExperimentIdClientPrivacy, @@ -165,7 +155,6 @@ enum ExperimentIds { kExperimentIdMaxPingsWoDataThrottle, kExperimentIdMonitoringExperiment, kExperimentIdMultiping, - kExperimentIdPeerStateBasedFraming, kExperimentIdPickFirstNew, kExperimentIdPromiseBasedInprocTransport, kExperimentIdScheduleCancellationOverWrite, @@ -178,10 +167,6 @@ enum ExperimentIds { kExperimentIdWorkSerializerDispatch, kNumExperiments }; -#define GRPC_EXPERIMENT_IS_INCLUDED_CALL_STATUS_OVERRIDE_ON_CANCELLATION -inline bool IsCallStatusOverrideOnCancellationEnabled() { - return IsExperimentEnabled(); -} #define GRPC_EXPERIMENT_IS_INCLUDED_CALL_TRACER_IN_TRANSPORT inline bool IsCallTracerInTransportEnabled() { return IsExperimentEnabled(); @@ -222,10 +207,6 @@ inline bool IsMonitoringExperimentEnabled() { inline bool IsMultipingEnabled() { return IsExperimentEnabled(); } -#define GRPC_EXPERIMENT_IS_INCLUDED_PEER_STATE_BASED_FRAMING -inline bool IsPeerStateBasedFramingEnabled() { - return IsExperimentEnabled(); -} #define GRPC_EXPERIMENT_IS_INCLUDED_PICK_FIRST_NEW inline bool IsPickFirstNewEnabled() { return IsExperimentEnabled(); diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index 8179377da0e08..a244a416e98b8 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -40,13 +40,6 @@ # This file only defines the experiments. Refer to rollouts.yaml for the rollout # state of each experiment. -- name: call_status_override_on_cancellation - description: - Avoid overriding call status of successfully finished calls if it races with - cancellation. - expiry: 2024/08/01 - owner: vigneshbabu@google.com - test_tags: [] - name: call_tracer_in_transport description: Transport directly passes byte counts to CallTracer. expiry: 2024/09/30 @@ -82,7 +75,7 @@ uses_polling: true - name: event_engine_listener description: Use EventEngine listeners instead of iomgr's grpc_tcp_server - expiry: 2024/10/01 + expiry: 2024/12/01 owner: vigneshbabu@google.com test_tags: ["core_end2end_test", "event_engine_listener_test"] uses_polling: true @@ -110,14 +103,6 @@ expiry: 2024/09/15 owner: ctiller@google.com test_tags: [flow_control_test] -- name: peer_state_based_framing - description: - If set, the max sizes of frames sent to lower layers is controlled based - on the peer's memory pressure which is reflected in its max http2 frame - size. - expiry: 2024/08/01 - owner: vigneshbabu@google.com - test_tags: ["flow_control_test"] - name: pick_first_new description: New pick_first impl with memory reduction. expiry: 2024/07/30 @@ -132,7 +117,7 @@ allow_in_fuzzing_config: false # experiment currently crashes if enabled - name: schedule_cancellation_over_write description: Allow cancellation op to be scheduled over a write - expiry: 2024/08/01 + expiry: 2024/12/01 owner: vigneshbabu@google.com test_tags: [] - name: server_privacy @@ -148,17 +133,17 @@ TCP would not indicate completion of a read operation until a specified number of bytes have been read over the socket. Buffers are also allocated according to estimated RPC sizes. - expiry: 2024/08/01 + expiry: 2024/12/01 owner: vigneshbabu@google.com test_tags: ["endpoint_test", "flow_control_test"] - name: tcp_rcv_lowat description: Use SO_RCVLOWAT to avoid wakeups on the read path. - expiry: 2024/08/01 + expiry: 2024/12/01 owner: vigneshbabu@google.com test_tags: ["endpoint_test", "flow_control_test"] - name: trace_record_callops description: Enables tracing of call batch initiation and completion. - expiry: 2024/08/01 + expiry: 2024/12/01 owner: vigneshbabu@google.com test_tags: [] - name: unconstrained_max_quota_buffer_size diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index a8803b2331afd..b2a62d4bb9b15 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -40,8 +40,6 @@ # # Supported platforms: ios, windows, posix -- name: call_status_override_on_cancellation - default: true - name: call_tracer_in_transport default: true - name: call_v3 @@ -79,8 +77,6 @@ default: false - name: monitoring_experiment default: true -- name: peer_state_based_framing - default: false - name: pending_queue_cap default: true - name: pick_first_new diff --git a/src/core/lib/surface/filter_stack_call.cc b/src/core/lib/surface/filter_stack_call.cc index 17d8fe216673f..a3a6e3d7b6014 100644 --- a/src/core/lib/surface/filter_stack_call.cc +++ b/src/core/lib/surface/filter_stack_call.cc @@ -533,15 +533,13 @@ void FilterStackCall::BatchControl::PostCompletion() { FilterStackCall* call = call_; grpc_error_handle error = batch_error_.get(); - if (IsCallStatusOverrideOnCancellationEnabled()) { - // On the client side, if final call status is already known (i.e if this op - // includes recv_trailing_metadata) and if the call status is known to be - // OK, then disregard the batch error to ensure call->receiving_buffer_ is - // not cleared. - if (op_.recv_trailing_metadata && call->is_client() && - call->status_error_.ok()) { - error = absl::OkStatus(); - } + // On the client side, if final call status is already known (i.e if this op + // includes recv_trailing_metadata) and if the call status is known to be + // OK, then disregard the batch error to ensure call->receiving_buffer_ is + // not cleared. + if (op_.recv_trailing_metadata && call->is_client() && + call->status_error_.ok()) { + error = absl::OkStatus(); } GRPC_TRACE_VLOG(call, 2) << "tag:" << completion_data_.notify_tag.tag