reviewer feedback

6 files changed, 18 insertions, 23 deletions

diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index bb79005654..375cf25cc6 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 9a448d3b38..603ec0b473 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 826c0fb834..bff3ec379f 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<int>(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 815d302577..52056b8671 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 f947617d0f..4bfab4e2ee 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<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE),
-        max_trace_nodes);
-    server_a[1] = grpc_channel_arg_integer_create(
-        const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ), true);
+  explicit ServerFixture(int max_trace_nodes = 0) {
+    grpc_arg server_a[] = {
+        grpc_channel_arg_integer_create(
+            const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE),
+            max_trace_nodes),
+        grpc_channel_arg_integer_create(
+            const_cast<char*>(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 f96c430b69..40a0370f0e 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\""));