From fe1f7f58139a3ccafb7b8e8ca92d5d209880a3e5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 7 Sep 2018 15:58:22 -0700 Subject: [PATCH] reviewer feedback --- src/core/lib/channel/channelz.cc | 3 --- src/core/lib/channel/channelz.h | 2 -- src/core/lib/surface/call.cc | 11 +++++------ src/core/lib/surface/channel.cc | 4 ++-- test/core/channel/channelz_test.cc | 17 +++++++++-------- test/core/end2end/tests/channelz.cc | 4 ++-- 6 files changed, 18 insertions(+), 23 deletions(-) diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index bb790056544..375cf25cc66 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -181,9 +181,6 @@ grpc_json* ServerNode::RenderJson() { // ask CallCountingHelper to populate trace and call count data. call_counter_.PopulateCallCounts(json); json = top_level_json; - // template method. Child classes may override this to add their specific - // functionality. - PopulateSockets(json); return top_level_json; } diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 9a448d3b389..603ec0b4732 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -174,8 +174,6 @@ class ServerNode : public BaseNode { grpc_json* RenderJson() override; - void PopulateSockets(grpc_json* json) {} - // proxy methods to composed classes. void AddTraceEvent(ChannelTrace::Severity severity, grpc_slice data) { trace_.AddTraceEvent(severity, data); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 826c0fb8348..bff3ec379f8 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -167,8 +167,6 @@ struct grpc_call { grpc_completion_queue* cq; grpc_polling_entity pollent; grpc_channel* channel; - // backpointer to owning server if this is a server side call. - grpc_server* server; gpr_timespec start_time; /* parent_call* */ gpr_atm parent_call_atm; child_call* child; @@ -250,6 +248,8 @@ struct grpc_call { } client; struct { int* cancelled; + // backpointer to owning server if this is a server side call. + grpc_server* server; } server; } final_op; @@ -369,7 +369,6 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, grpc_slice path = grpc_empty_slice(); if (call->is_client) { GRPC_STATS_INC_CLIENT_CALLS_CREATED(); - call->server = nullptr; GPR_ASSERT(args->add_initial_metadata_count < MAX_SEND_EXTRA_METADATA_COUNT); for (i = 0; i < args->add_initial_metadata_count; i++) { @@ -384,7 +383,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, static_cast(args->add_initial_metadata_count); } else { GRPC_STATS_INC_SERVER_CALLS_CREATED(); - call->server = args->server; + call->final_op.server.server = args->server; GPR_ASSERT(args->add_initial_metadata_count == 0); call->send_extra_metadata_count = 0; } @@ -496,7 +495,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, } } else { grpc_core::channelz::ServerNode* channelz_server = - grpc_server_get_channelz_node(call->server); + grpc_server_get_channelz_node(call->final_op.server.server); if (channelz_server != nullptr) { channelz_server->RecordCallStarted(); } @@ -1286,7 +1285,7 @@ static void post_batch_completion(batch_control* bctl) { get_final_status(call, set_cancelled_value, call->final_op.server.cancelled, nullptr, nullptr); grpc_core::channelz::ServerNode* channelz_server = - grpc_server_get_channelz_node(call->server); + grpc_server_get_channelz_node(call->final_op.server.server); if (channelz_server != nullptr) { if (*call->final_op.server.cancelled) { channelz_server->RecordCallFailed(); diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 815d302577a..52056b86711 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -166,8 +166,8 @@ grpc_channel* grpc_channel_create_with_builder( } grpc_channel_args_destroy(args); - // only track client channels since server channels are held in the - // grpc_server channel. + // we only need to do the channelz bookkeeping for clients here. The channelz + // bookkeeping for server channels occurs in src/core/lib/surface/server.cc if (channelz_enabled && channel->is_client) { channel->channelz_channel = channel_node_create_func( channel, channel_tracer_max_nodes, !internal_channel); diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index f947617d0fd..4bfab4e2eec 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -146,20 +146,21 @@ class ChannelFixture { class ServerFixture { public: - ServerFixture(int max_trace_nodes = 0) { - grpc_arg server_a[2]; - server_a[0] = grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE), - max_trace_nodes); - server_a[1] = grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_ENABLE_CHANNELZ), true); + explicit ServerFixture(int max_trace_nodes = 0) { + grpc_arg server_a[] = { + grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE), + max_trace_nodes), + grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), true), + }; grpc_channel_args server_args = {GPR_ARRAY_SIZE(server_a), server_a}; server_ = grpc_server_create(&server_args, nullptr); } ~ServerFixture() { grpc_server_destroy(server_); } - grpc_server* server() { return server_; } + grpc_server* server() const { return server_; } private: grpc_server* server_; diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index f96c430b69f..40a0370f0ea 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -240,7 +240,7 @@ static void test_channelz(grpc_end2end_test_config config) { GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"2\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\"")); - // channel tracing is not enables, so these should not be preset. + // channel tracing is not enabled, so these should not be preset. GPR_ASSERT(nullptr == strstr(json, "\"trace\"")); GPR_ASSERT(nullptr == strstr(json, "\"description\":\"Channel created\"")); GPR_ASSERT(nullptr == strstr(json, "\"severity\":\"CT_INFO\"")); @@ -252,7 +252,7 @@ static void test_channelz(grpc_end2end_test_config config) { GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"2\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\"")); - // channel tracing is not enables, so these should not be preset. + // channel tracing is not enabled, so these should not be preset. GPR_ASSERT(nullptr == strstr(json, "\"trace\"")); GPR_ASSERT(nullptr == strstr(json, "\"description\":\"Channel created\"")); GPR_ASSERT(nullptr == strstr(json, "\"severity\":\"CT_INFO\""));