From 25082c5fc47d0b337657476a0fa7ce989b9712ef Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 12 Jul 2018 17:24:48 -0700 Subject: [PATCH] Reviewer feedback --- include/grpc/support/string_util.h | 2 -- .../filters/client_channel/client_channel.cc | 2 +- .../filters/client_channel/client_channel.h | 1 - .../ext/filters/client_channel/lb_policy.cc | 12 ---------- .../ext/filters/client_channel/lb_policy.h | 16 ++++++------- .../lb_policy/pick_first/pick_first.cc | 24 +++++++------------ src/core/lib/channel/channelz.cc | 1 - src/core/lib/gpr/string.cc | 6 ----- src/core/lib/gpr/string.h | 10 ++++++++ src/core/lib/gprpp/abstract.h | 7 ++++-- src/core/lib/iomgr/error.cc | 3 +-- src/core/lib/json/json.h | 3 ++- 12 files changed, 36 insertions(+), 51 deletions(-) diff --git a/include/grpc/support/string_util.h b/include/grpc/support/string_util.h index 3b5b0008b24..2679160c1bb 100644 --- a/include/grpc/support/string_util.h +++ b/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 diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 8c3e9f2b305..ca770b2f1a3 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/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(elem->channel_data); - if (chand->lb_policy) { + if (chand->lb_policy != nullptr) { chand->lb_policy->FillChildRefsForChannelz(child_subchannels, child_channels); } diff --git a/src/core/ext/filters/client_channel/client_channel.h b/src/core/ext/filters/client_channel/client_channel.h index 0f95f6670a6..0b44a17562f 100644 --- a/src/core/ext/filters/client_channel/client_channel.h +++ b/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; diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc index a0d7758f235..d7b3ff6bb61 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/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) { diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index 0c5fd2ad9cf..e756c89208f 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/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 diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index e43d1c2ab46..ab56922aa58 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/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(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() || diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index ffc7ff884cd..d9bce986d44 100644 --- a/src/core/lib/channel/channelz.cc +++ b/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); diff --git a/src/core/lib/gpr/string.cc b/src/core/lib/gpr/string.cc index 9bcb6714a39..0a76fc1f54a 100644 --- a/src/core/lib/gpr/string.cc +++ b/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 diff --git a/src/core/lib/gpr/string.h b/src/core/lib/gpr/string.h index 2e8a4898d9d..ce51fe46321 100644 --- a/src/core/lib/gpr/string.h +++ b/src/core/lib/gpr/string.h @@ -21,6 +21,8 @@ #include +#include + #include #include @@ -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; diff --git a/src/core/lib/gprpp/abstract.h b/src/core/lib/gprpp/abstract.h index cc96edc49b7..5b7018e07e0 100644 --- a/src/core/lib/gprpp/abstract.h +++ b/src/core/lib/gprpp/abstract.h @@ -28,7 +28,10 @@ // 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); } +#define GRPC_ABSTRACT \ + { \ + gpr_log(GPR_ERROR, "Function marked GRPC_ABSTRACT was not implemented"); \ + GPR_ASSERT(false); \ + } #endif /* GRPC_CORE_LIB_GPRPP_ABSTRACT_H */ diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index 8e7205f5586..90ed34da11f 100644 --- a/src/core/lib/iomgr/error.cc +++ b/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(which)), - gpr_format_timespec( - *reinterpret_cast(err->arena + slot))); + fmt_time(*reinterpret_cast(err->arena + slot))); } } } diff --git a/src/core/lib/json/json.h b/src/core/lib/json/json.h index 8742774b5fc..8173845c723 100644 --- a/src/core/lib/json/json.h +++ b/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);