From 97f22ce29528927b98b8494b8e7338d943b6487e Mon Sep 17 00:00:00 2001 From: Tanvi Jagtap <139093547+tanvi-jagtap@users.noreply.github.com> Date: Mon, 23 Sep 2024 23:31:52 -0700 Subject: [PATCH] [PH2][Refactor][is_client] Split functions (#37641) [PH2][Refactor][is_client] Split two functions for better readability 1. send_initial_metadata_locked - Moved out the tracing part 2. grpc_chttp2_config_default_keepalive_args - Split the client and server path to reduce cyclomatic complexity. Closes #37641 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37641 from tanvi-jagtap:remove_is_client_02 bfbc9361dbcbc78282e4e0c7a5d4b41b9f53f30c PiperOrigin-RevId: 678096501 --- .../chttp2/transport/chttp2_transport.cc | 70 ++++++++++--------- 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index f25e8b2c672..02fffbf41ca 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1361,10 +1361,7 @@ static void log_metadata(const grpc_metadata_batch* md_batch, uint32_t id, }); } -static void send_initial_metadata_locked( - grpc_transport_stream_op_batch* op, grpc_chttp2_stream* s, - grpc_transport_stream_op_batch_payload* op_payload, - grpc_chttp2_transport* t, grpc_closure* on_complete) { +static void trace_annotations(grpc_chttp2_stream* s) { if (!grpc_core::IsCallTracerInTransportEnabled()) { if (s->call_tracer != nullptr) { s->call_tracer->RecordAnnotation( @@ -1383,6 +1380,13 @@ static void send_initial_metadata_locked( .Add(s->flow_control.stats())); } } +} + +static void send_initial_metadata_locked( + grpc_transport_stream_op_batch* op, grpc_chttp2_stream* s, + grpc_transport_stream_op_batch_payload* op_payload, + grpc_chttp2_transport* t, grpc_closure* on_complete) { + trace_annotations(s); if (t->is_client && t->channelz_socket != nullptr) { t->channelz_socket->RecordStreamStartedFromLocal(); } @@ -2915,48 +2919,48 @@ static void next_bdp_ping_timer_expired_locked( } void grpc_chttp2_config_default_keepalive_args(grpc_channel_args* args, - bool is_client) { + const bool is_client) { grpc_chttp2_config_default_keepalive_args(grpc_core::ChannelArgs::FromC(args), is_client); } -void grpc_chttp2_config_default_keepalive_args( - const grpc_core::ChannelArgs& channel_args, bool is_client) { - const auto keepalive_time = +static void grpc_chttp2_config_default_keepalive_args_client( + const grpc_core::ChannelArgs& channel_args) { + g_default_client_keepalive_time = std::max(grpc_core::Duration::Milliseconds(1), channel_args.GetDurationFromIntMillis(GRPC_ARG_KEEPALIVE_TIME_MS) - .value_or(is_client ? g_default_client_keepalive_time - : g_default_server_keepalive_time)); - if (is_client) { - g_default_client_keepalive_time = keepalive_time; - } else { - g_default_server_keepalive_time = keepalive_time; - } - - const auto keepalive_timeout = std::max( + .value_or(g_default_client_keepalive_time)); + g_default_client_keepalive_timeout = std::max( grpc_core::Duration::Zero(), channel_args.GetDurationFromIntMillis(GRPC_ARG_KEEPALIVE_TIMEOUT_MS) - .value_or(is_client ? g_default_client_keepalive_timeout - : g_default_server_keepalive_timeout)); - if (is_client) { - g_default_client_keepalive_timeout = keepalive_timeout; - } else { - g_default_server_keepalive_timeout = keepalive_timeout; - } + .value_or(g_default_client_keepalive_timeout)); + g_default_client_keepalive_permit_without_calls = + channel_args.GetBool(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS) + .value_or(g_default_client_keepalive_permit_without_calls); +} - const bool keepalive_permit_without_calls = +static void grpc_chttp2_config_default_keepalive_args_server( + const grpc_core::ChannelArgs& channel_args) { + g_default_server_keepalive_time = + std::max(grpc_core::Duration::Milliseconds(1), + channel_args.GetDurationFromIntMillis(GRPC_ARG_KEEPALIVE_TIME_MS) + .value_or(g_default_server_keepalive_time)); + g_default_server_keepalive_timeout = std::max( + grpc_core::Duration::Zero(), + channel_args.GetDurationFromIntMillis(GRPC_ARG_KEEPALIVE_TIMEOUT_MS) + .value_or(g_default_server_keepalive_timeout)); + g_default_server_keepalive_permit_without_calls = channel_args.GetBool(GRPC_ARG_KEEPALIVE_PERMIT_WITHOUT_CALLS) - .value_or(is_client - ? g_default_client_keepalive_permit_without_calls - : g_default_server_keepalive_permit_without_calls); + .value_or(g_default_server_keepalive_permit_without_calls); +} + +void grpc_chttp2_config_default_keepalive_args( + const grpc_core::ChannelArgs& channel_args, const bool is_client) { if (is_client) { - g_default_client_keepalive_permit_without_calls = - keepalive_permit_without_calls; + grpc_chttp2_config_default_keepalive_args_client(channel_args); } else { - g_default_server_keepalive_permit_without_calls = - keepalive_permit_without_calls; + grpc_chttp2_config_default_keepalive_args_server(channel_args); } - grpc_core::Chttp2PingAbusePolicy::SetDefaults(channel_args); grpc_core::Chttp2PingRatePolicy::SetDefaults(channel_args); }