reviewer feedback

src/core/lib/channel/channel_trace.cc

@@ -70,7 +70,7 @@ ChannelTrace::ChannelTrace(size_t max_events)
       tail_trace_(nullptr) {
   if (max_list_size_ == 0) return;  // tracing is disabled if max_events == 0
   gpr_mu_init(&tracer_mu_);
-  channel_uuid_ = ChannelzRegistry::Default()->Register(this);
+  channel_uuid_ = ChannelzRegistry::Register(this);
   time_created_ = grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(),
                                           GPR_CLOCK_REALTIME);
 }
@@ -83,7 +83,7 @@ ChannelTrace::~ChannelTrace() {
     it = it->next();
     Delete<TraceEvent>(to_free);
   }
-  ChannelzRegistry::Default()->Unregister(channel_uuid_);
+  ChannelzRegistry::Unregister(channel_uuid_);
   gpr_mu_destroy(&tracer_mu_);
 }
 

src/core/lib/channel/channelz_registry.cc

@@ -68,7 +68,7 @@ ChannelzRegistry::~ChannelzRegistry() {
   gpr_mu_destroy(&mu_);
 }
 
-void ChannelzRegistry::Unregister(intptr_t uuid) {
+void ChannelzRegistry::InternalUnregister(intptr_t uuid) {
   gpr_mu_lock(&mu_);
   avl_ = grpc_avl_remove(avl_, (void*)uuid, nullptr);
   gpr_mu_unlock(&mu_);

src/core/lib/channel/channelz_registry.h

@@ -32,22 +32,41 @@ namespace grpc_core {
 // channelz bookkeeping. All objects share globally distributed uuids.
 class ChannelzRegistry {
  public:
-  // These functions ensure singleton like behavior. We cannot use the normal
-  // pattern of a get functions with a static function variable due to build
-  // complications.
-
   // To be called in grpc_init()
   static void Init();
 
   // To be callen in grpc_shutdown();
   static void Shutdown();
 
+  // globally registers a channelz Object. Returns its unique uuid
+  template <typename Object>
+  static intptr_t Register(Object* object) {
+    return Default()->InternalRegister(object);
+  }
+
+  // globally unregisters the object that is associated to uuid.
+  static void Unregister(intptr_t uuid) { Default()->InternalUnregister(uuid); }
+
+  // if object with uuid has previously been registered, returns the
+  // Object associated with that uuid. Else returns nullptr.
+  template <typename Object>
+  static Object* Get(intptr_t uuid) {
+    return Default()->InternalGet<Object>(uuid);
+  }
+
+ private:
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
+  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
+
+  ChannelzRegistry();
+  ~ChannelzRegistry();
+
   // Returned the singleton instance of ChannelzRegistry;
   static ChannelzRegistry* Default();
 
   // globally registers a channelz Object. Returns its unique uuid
   template <typename Object>
-  intptr_t Register(Object* object) {
+  intptr_t InternalRegister(Object* object) {
     intptr_t prior = gpr_atm_no_barrier_fetch_add(&uuid_, 1);
     gpr_mu_lock(&mu_);
     avl_ = grpc_avl_add(avl_, (void*)prior, object, nullptr);
@@ -56,12 +75,12 @@ class ChannelzRegistry {
   }
 
   // globally unregisters the object that is associated to uuid.
-  void Unregister(intptr_t uuid);
+  void InternalUnregister(intptr_t uuid);
 
   // if object with uuid has previously been registered, returns the
   // Object associated with that uuid. Else returns nullptr.
   template <typename Object>
-  Object* Get(intptr_t uuid) {
+  Object* InternalGet(intptr_t uuid) {
     gpr_mu_lock(&mu_);
     Object* ret =
         static_cast<Object*>(grpc_avl_get(avl_, (void*)uuid, nullptr));
@@ -69,16 +88,10 @@ class ChannelzRegistry {
     return ret;
   }
 
- private:
-  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
-
+  // private members
   gpr_mu mu_;
   grpc_avl avl_;
   gpr_atm uuid_;
-
-  ChannelzRegistry();
-  ~ChannelzRegistry();
 };
 
 }  // namespace grpc_core

test/core/channel/channel_trace_test.cc

@@ -99,8 +99,7 @@ void ValidateTraceDataMatchedUuidLookup(RefCountedPtr<ChannelTrace> tracer) {
   intptr_t uuid = tracer->GetUuid();
   if (uuid == -1) return;  // Doesn't make sense to lookup if tracing disabled
   char* tracer_json_str = tracer->RenderTrace();
-  ChannelTrace* uuid_lookup =
-      ChannelzRegistry::Default()->Get<ChannelTrace>(uuid);
+  ChannelTrace* uuid_lookup = ChannelzRegistry::Get<ChannelTrace>(uuid);
   char* uuid_lookup_json_str = uuid_lookup->RenderTrace();
   EXPECT_EQ(strcmp(tracer_json_str, uuid_lookup_json_str), 0);
   gpr_free(tracer_json_str);

test/core/channel/channelz_registry_test.cc

@@ -43,12 +43,12 @@ namespace testing {
 // lookups by uuid.
 TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) {
   int object_to_register;
-  intptr_t uuid = ChannelzRegistry::Default()->Register(&object_to_register);
-  ASSERT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if "
+  intptr_t uuid = ChannelzRegistry::Register(&object_to_register);
+  EXPECT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if "
                         "reserved according to "
                         "https://github.com/grpc/proposal/blob/master/"
                         "A14-channelz.md";
-  ChannelzRegistry::Default()->Unregister(uuid);
+  ChannelzRegistry::Unregister(uuid);
 }
 
 TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
@@ -56,33 +56,29 @@ TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
   std::vector<intptr_t> uuids;
   for (int i = 0; i < 10; ++i) {
     // reregister the same object. It's ok since we are just testing uuids
-    uuids.push_back(ChannelzRegistry::Default()->Register(&object_to_register));
+    uuids.push_back(ChannelzRegistry::Register(&object_to_register));
   }
   for (size_t i = 1; i < uuids.size(); ++i) {
-    ASSERT_LT(uuids[i - 1], uuids[i]) << "Uuids must always be increasing";
+    EXPECT_LT(uuids[i - 1], uuids[i]) << "Uuids must always be increasing";
   }
 }
 
 TEST(ChannelzRegistryTest, RegisterGetTest) {
   int object_to_register = 42;
-  intptr_t uuid = ChannelzRegistry::Default()->Register(&object_to_register);
-  int* retrieved = ChannelzRegistry::Default()->Get<int>(uuid);
-  ASSERT_EQ(object_to_register, *retrieved);
-  ASSERT_EQ(&object_to_register, retrieved);
+  intptr_t uuid = ChannelzRegistry::Register(&object_to_register);
+  int* retrieved = ChannelzRegistry::Get<int>(uuid);
+  EXPECT_EQ(&object_to_register, retrieved);
 }
 
 TEST(ChannelzRegistryTest, MultipleTypeTest) {
   int int_to_register = 42;
-  intptr_t int_uuid = ChannelzRegistry::Default()->Register(&int_to_register);
+  intptr_t int_uuid = ChannelzRegistry::Register(&int_to_register);
   std::string str_to_register = "hello world";
-  intptr_t str_uuid = ChannelzRegistry::Default()->Register(&str_to_register);
-  int* retrieved_int = ChannelzRegistry::Default()->Get<int>(int_uuid);
-  std::string* retrieved_str =
-      ChannelzRegistry::Default()->Get<std::string>(str_uuid);
-  ASSERT_EQ(int_to_register, *retrieved_int);
-  ASSERT_EQ(&int_to_register, retrieved_int);
-  ASSERT_STREQ(str_to_register.c_str(), (*retrieved_str).c_str());
-  ASSERT_EQ(&str_to_register, retrieved_str);
+  intptr_t str_uuid = ChannelzRegistry::Register(&str_to_register);
+  int* retrieved_int = ChannelzRegistry::Get<int>(int_uuid);
+  std::string* retrieved_str = ChannelzRegistry::Get<std::string>(str_uuid);
+  EXPECT_EQ(&int_to_register, retrieved_int);
+  EXPECT_EQ(&str_to_register, retrieved_str);
 }
 
 namespace {
@@ -95,21 +91,20 @@ class Foo {
 TEST(ChannelzRegistryTest, CustomObjectTest) {
   Foo* foo = New<Foo>();
   foo->bar = 1024;
-  intptr_t uuid = ChannelzRegistry::Default()->Register(foo);
-  Foo* retrieved = ChannelzRegistry::Default()->Get<Foo>(uuid);
-  ASSERT_EQ(foo, retrieved);
-  ASSERT_EQ(foo->bar, retrieved->bar);
+  intptr_t uuid = ChannelzRegistry::Register(foo);
+  Foo* retrieved = ChannelzRegistry::Get<Foo>(uuid);
+  EXPECT_EQ(foo, retrieved);
 }
 
 TEST(ChannelzRegistryTest, NullIfNotPresentTest) {
   int object_to_register = 42;
-  intptr_t uuid = ChannelzRegistry::Default()->Register(&object_to_register);
+  intptr_t uuid = ChannelzRegistry::Register(&object_to_register);
   // try to pull out a uuid that does not exist.
-  int* nonexistant = ChannelzRegistry::Default()->Get<int>(1234);
-  ASSERT_EQ(nonexistant, nullptr);
-  int* retrieved = ChannelzRegistry::Default()->Get<int>(uuid);
-  ASSERT_EQ(object_to_register, *retrieved);
-  ASSERT_EQ(&object_to_register, retrieved);
+  int* nonexistant = ChannelzRegistry::Get<int>(uuid + 1);
+  EXPECT_EQ(nonexistant, nullptr);
+  int* retrieved = ChannelzRegistry::Get<int>(uuid);
+  EXPECT_EQ(object_to_register, *retrieved);
+  EXPECT_EQ(&object_to_register, retrieved);
 }
 
 }  // namespace testing