Reviewer feedback

include/grpc/support/string_util.h

@@ -44,8 +44,6 @@ GPRAPI char* gpr_strdup(const char* src);
 GPRAPI int gpr_asprintf(char** strp, const char* format, ...)
     GPR_PRINT_FORMAT_CHECK(2, 3);
 
-GPRAPI char* gpr_format_timespec(gpr_timespec);
-
 #ifdef __cplusplus
 }
 #endif

src/core/ext/filters/client_channel/client_channel.cc

@@ -3163,7 +3163,7 @@ void grpc_client_channel_populate_child_refs(
     grpc_channel_element* elem, grpc_core::ChildRefsList* child_subchannels,
     grpc_core::ChildRefsList* child_channels) {
   channel_data* chand = static_cast<channel_data*>(elem->channel_data);
-  if (chand->lb_policy) {
+  if (chand->lb_policy != nullptr) {
     chand->lb_policy->FillChildRefsForChannelz(child_subchannels,
                                                child_channels);
   }

src/core/ext/filters/client_channel/client_channel.h

@@ -25,7 +25,6 @@
 #include "src/core/ext/filters/client_channel/client_channel_factory.h"
 #include "src/core/ext/filters/client_channel/resolver.h"
 #include "src/core/lib/channel/channel_stack.h"
-#include "src/core/lib/json/json.h"
 
 extern grpc_core::TraceFlag grpc_client_channel_trace;
 

src/core/ext/filters/client_channel/lb_policy.cc

@@ -41,18 +41,6 @@ LoadBalancingPolicy::~LoadBalancingPolicy() {
   GRPC_COMBINER_UNREF(combiner_, "lb_policy");
 }
 
-void LoadBalancingPolicy::FillChildRefsForChannelz(
-    ChildRefsList* child_subchannels, ChildRefsList* child_channels) {
-  mu_guard guard(&child_refs_mu_);
-  // TODO, de dup these.
-  for (size_t i = 0; i < child_subchannels_.size(); ++i) {
-    child_subchannels->push_back(child_subchannels_[i]);
-  }
-  for (size_t i = 0; i < child_channels_.size(); ++i) {
-    child_channels->push_back(child_channels_[i]);
-  }
-}
-
 void LoadBalancingPolicy::TryReresolutionLocked(
     grpc_core::TraceFlag* grpc_lb_trace, grpc_error* error) {
   if (request_reresolution_ != nullptr) {

src/core/ext/filters/client_channel/lb_policy.h

@@ -29,7 +29,6 @@
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
 #include "src/core/lib/iomgr/combiner.h"
 #include "src/core/lib/iomgr/polling_entity.h"
-#include "src/core/lib/json/json.h"
 #include "src/core/lib/transport/connectivity_state.h"
 
 extern grpc_core::DebugOnlyTraceFlag grpc_trace_lb_policy_refcount;
@@ -145,6 +144,14 @@ class LoadBalancingPolicy
   /// consider whether this method is still needed.
   virtual void ExitIdleLocked() GRPC_ABSTRACT;
 
+  /// populates child_subchannels and child_channels with the uuids of this
+  /// LB policy's referenced children. This is not invoked from the
+  /// client_channel's combiner. The implementation is responsible for
+  /// providing its own synchronization.
+  virtual void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
+                                        ChildRefsList* child_channels)
+      GRPC_ABSTRACT;
+
   void Orphan() override {
     // Invoke ShutdownAndUnrefLocked() inside of the combiner.
     GRPC_CLOSURE_SCHED(
@@ -159,13 +166,6 @@ class LoadBalancingPolicy
     request_reresolution_ = request_reresolution;
   }
 
-  /// populates child_subchannels and child_channels with the uuids of this
-  /// LB policies referenced children. This is not invoked from the
-  /// client_channel's combiner. It has its own synchronization. This is
-  /// not abstract, since the behavior is the same for all LB policies.
-  void FillChildRefsForChannelz(ChildRefsList* child_subchannels,
-                                ChildRefsList* child_channels);
-
   grpc_pollset_set* interested_parties() const { return interested_parties_; }
 
   GRPC_ABSTRACT_BASE_CLASS

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc

@@ -37,15 +37,6 @@ TraceFlag grpc_lb_pick_first_trace(false, "pick_first");
 
 namespace {
 
-class LockGuard {
- public:
-  LockGuard(gpr_mu* mu) : mu_(mu) { gpr_mu_lock(mu_); }
-  ~LockGuard() { gpr_mu_unlock(mu_); }
-
- private:
-  gpr_mu* mu_;
-};
-
 //
 // pick_first LB policy
 //
@@ -112,10 +103,13 @@ class PickFirst : public LoadBalancingPolicy {
     }
   };
 
-  class UpdateGuard {
+  // Helper class to ensure that any function that modifies the child refs
+  // data structures will update the channelz snapshot data structures before
+  // returning.
+  class AutoChildRefsUpdater {
    public:
-    UpdateGuard(PickFirst* pf) : pf_(pf) {}
-    ~UpdateGuard() { pf_->UpdateChildRefsLocked(); }
+    explicit AutoChildRefsUpdater(PickFirst* pf) : pf_(pf) {}
+    ~AutoChildRefsUpdater() { pf_->UpdateChildRefsLocked(); }
 
    private:
     PickFirst* pf_;
@@ -177,7 +171,7 @@ void PickFirst::HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) {
 }
 
 void PickFirst::ShutdownLocked() {
-  UpdateGuard(this);
+  AutoChildRefsUpdater(this);
   grpc_error* error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel shutdown");
   if (grpc_lb_pick_first_trace.enabled()) {
     gpr_log(GPR_INFO, "Pick First %p Shutting down", this);
@@ -330,7 +324,7 @@ void PickFirst::UpdateChildRefsLocked() {
 }
 
 void PickFirst::UpdateLocked(const grpc_channel_args& args) {
-  UpdateGuard guard(this);
+  AutoChildRefsUpdater guard(this);
   const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES);
   if (arg == nullptr || arg->type != GRPC_ARG_POINTER) {
     if (subchannel_list_ == nullptr) {
@@ -438,7 +432,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) {
 void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked(
     grpc_connectivity_state connectivity_state, grpc_error* error) {
   PickFirst* p = static_cast<PickFirst*>(subchannel_list()->policy());
-  UpdateGuard guard(p);
+  AutoChildRefsUpdater guard(p);
   // The notification must be for a subchannel in either the current or
   // latest pending subchannel lists.
   GPR_ASSERT(subchannel_list() == p->subchannel_list_.get() ||

src/core/lib/channel/channelz.cc

@@ -115,7 +115,6 @@ char* ChannelNode::RenderJSON() {
   json_iterator =
       grpc_json_create_child(json_iterator, json, "lastCallStartedTimestamp",
                              gpr_format_timespec(ts), GRPC_JSON_STRING, true);
-
   json = top_level_json;
   json_iterator = nullptr;
   PopulateChildRefs(json);

src/core/lib/gpr/string.cc

@@ -56,12 +56,6 @@ typedef struct {
   char* data;
 } dump_out;
 
-// Returns an allocated string that represents tm according to RFC-3339, and,
-// more specifically, follows:
-// https://developers.google.com/protocol-buffers/docs/proto3#json
-//
-// "Uses RFC 3339, where generated output will always be Z-normalized and uses
-// 0, 3, 6 or 9 fractional digits."
 char* gpr_format_timespec(gpr_timespec tm) {
   char time_buffer[35];
   char ns_buffer[11];  // '.' + 9 digits of precision

src/core/lib/gpr/string.h

@@ -21,6 +21,8 @@
 
 #include <grpc/support/port_platform.h>
 
+#include <grpc/impl/codegen/gpr_types.h>
+
 #include <stdbool.h>
 #include <stddef.h>
 
@@ -81,6 +83,14 @@ char* gpr_strjoin_sep(const char** strs, size_t nstrs, const char* sep,
 void gpr_string_split(const char* input, const char* sep, char*** strs,
                       size_t* nstrs);
 
+/* Returns an allocated string that represents tm according to RFC-3339, and,
+   more specifically, follows:
+   https://developers.google.com/protocol-buffers/docs/proto3#json
+
+   Uses RFC 3339, where generated output will always be Z-normalized and uses
+   0, 3, 6 or 9 fractional digits. */
+char* gpr_format_timespec(gpr_timespec);
+
 /* A vector of strings... for building up a final string one piece at a time */
 typedef struct {
   char** strs;

src/core/lib/gprpp/abstract.h

@@ -29,6 +29,9 @@
 // gRPC currently can't depend on libstdc++, so we can't use "= 0" for
 // pure virtual methods.  Instead, we use this macro.
 #define GRPC_ABSTRACT                                                        \
-  { GPR_ASSERT(false); }
+  {                                                                          \
+    gpr_log(GPR_ERROR, "Function marked GRPC_ABSTRACT was not implemented"); \
+    GPR_ASSERT(false);                                                       \
+  }
 
 #endif /* GRPC_CORE_LIB_GPRPP_ABSTRACT_H */

src/core/lib/iomgr/error.cc

@@ -670,8 +670,7 @@ static void collect_times_kvs(grpc_error* err, kv_pairs* kvs) {
     uint8_t slot = err->times[which];
     if (slot != UINT8_MAX) {
       append_kv(kvs, key_time(static_cast<grpc_error_times>(which)),
-                gpr_format_timespec(
-                    *reinterpret_cast<gpr_timespec*>(err->arena + slot)));
+                fmt_time(*reinterpret_cast<gpr_timespec*>(err->arena + slot)));
     }
   }
 }

src/core/lib/json/json.h

@@ -91,7 +91,8 @@ grpc_json* grpc_json_create_child(grpc_json* sibling, grpc_json* parent,
                                   const char* key, const char* value,
                                   grpc_json_type type, bool owns_value);
 
-/* TODO */
+/* Creates a child json string object from the integer num, then links the
+   json object into the parent's json tree */
 grpc_json* grpc_json_add_number_string_child(grpc_json* parent, grpc_json* it,
                                              const char* name, int64_t num);