From 82e9cb66ffb3fa3b7dc151efa056ded997186a59 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 12 Jul 2018 17:42:36 -0700 Subject: [PATCH] Reviewer feedback; move lists to children --- .../ext/filters/client_channel/lb_policy.cc | 5 +-- .../ext/filters/client_channel/lb_policy.h | 8 ----- .../client_channel/lb_policy/grpclb/grpclb.cc | 2 ++ .../lb_policy/pick_first/pick_first.cc | 32 ++++++++++++++++--- .../lb_policy/round_robin/round_robin.cc | 2 ++ 5 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc index d7b3ff6bb61..e065f45639b 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/src/core/ext/filters/client_channel/lb_policy.cc @@ -31,13 +31,10 @@ LoadBalancingPolicy::LoadBalancingPolicy(const Args& args) combiner_(GRPC_COMBINER_REF(args.combiner, "lb_policy")), client_channel_factory_(args.client_channel_factory), interested_parties_(grpc_pollset_set_create()), - request_reresolution_(nullptr) { - gpr_mu_init(&child_refs_mu_); -} + request_reresolution_(nullptr) {} LoadBalancingPolicy::~LoadBalancingPolicy() { grpc_pollset_set_destroy(interested_parties_); - gpr_mu_destroy(&child_refs_mu_); GRPC_COMBINER_UNREF(combiner_, "lb_policy"); } diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index e756c89208f..d525721ec03 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -180,9 +180,6 @@ class LoadBalancingPolicy grpc_client_channel_factory* client_channel_factory() const { return client_channel_factory_; } - gpr_mu* child_refs_mu() { return &child_refs_mu_; } - ChildRefsList* child_subchannels() { return &child_subchannels_; } - ChildRefsList* child_channels() { return &child_channels_; } /// Shuts down the policy. Any pending picks that have not been /// handed off to a new policy via HandOffPendingPicksLocked() will be @@ -202,11 +199,6 @@ class LoadBalancingPolicy /// Combiner under which LB policy actions take place. grpc_combiner* combiner_; - /// Lock and data used to capture snapshots of this channels child - /// channels and subchannels. This data is consumed by channelz. - gpr_mu child_refs_mu_; - ChildRefsList child_subchannels_; - ChildRefsList child_channels_; /// Client channel factory, used to create channels and subchannels. grpc_client_channel_factory* client_channel_factory_; /// Owned pointer to interested parties in load balancing decisions. diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 263b51ae895..622c03a8d15 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -135,6 +135,8 @@ class GrpcLb : public LoadBalancingPolicy { void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override; void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override; void ExitIdleLocked() override; + void FillChildRefsForChannelz(ChildRefsList* child_subchannels, + ChildRefsList* child_channels) override {} private: /// Linked list of pending pick requests. It stores all information needed to 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 ab56922aa58..7845c30f5b3 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 @@ -58,6 +58,8 @@ class PickFirst : public LoadBalancingPolicy { void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override; void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override; void ExitIdleLocked() override; + void FillChildRefsForChannelz(ChildRefsList* child_subchannels, + ChildRefsList* child_channels) override; private: ~PickFirst(); @@ -135,10 +137,17 @@ class PickFirst : public LoadBalancingPolicy { PickState* pending_picks_ = nullptr; // Our connectivity state tracker. grpc_connectivity_state_tracker state_tracker_; + + /// Lock and data used to capture snapshots of this channels child + /// channels and subchannels. This data is consumed by channelz. + gpr_mu child_refs_mu_; + ChildRefsList child_subchannels_; + ChildRefsList child_channels_; }; PickFirst::PickFirst(const Args& args) : LoadBalancingPolicy(args) { GPR_ASSERT(args.client_channel_factory != nullptr); + gpr_mu_init(&child_refs_mu_); grpc_connectivity_state_init(&state_tracker_, GRPC_CHANNEL_IDLE, "pick_first"); if (grpc_lb_pick_first_trace.enabled()) { @@ -152,6 +161,7 @@ PickFirst::~PickFirst() { if (grpc_lb_pick_first_trace.enabled()) { gpr_log(GPR_INFO, "Destroying Pick First %p", this); } + gpr_mu_destroy(&child_refs_mu_); GPR_ASSERT(subchannel_list_ == nullptr); GPR_ASSERT(latest_pending_subchannel_list_ == nullptr); GPR_ASSERT(pending_picks_ == nullptr); @@ -294,19 +304,31 @@ void PickFirst::PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) { } } +void PickFirst::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 PickFirst::UpdateChildRefsLocked() { - mu_guard guard(child_refs_mu()); + mu_guard guard(&child_refs_mu_); // reset both lists - child_subchannels()->clear(); + child_subchannels_.clear(); // this will stay empty, because pick_first channels have no children // channels. - child_channels()->clear(); + child_channels_.clear(); // populate the subchannels with boths subchannels lists, they will be // deduped when the actual channelz query comes in. if (subchannel_list_ != nullptr) { for (size_t i = 0; i < subchannel_list_->num_subchannels(); ++i) { if (subchannel_list_->subchannel(i)->subchannel() != nullptr) { - child_subchannels()->push_back(grpc_subchannel_get_uuid( + child_subchannels_.push_back(grpc_subchannel_get_uuid( subchannel_list_->subchannel(i)->subchannel())); } } @@ -316,7 +338,7 @@ void PickFirst::UpdateChildRefsLocked() { ++i) { if (latest_pending_subchannel_list_->subchannel(i)->subchannel() != nullptr) { - child_subchannels()->push_back(grpc_subchannel_get_uuid( + child_subchannels_.push_back(grpc_subchannel_get_uuid( latest_pending_subchannel_list_->subchannel(i)->subchannel())); } } diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index b1773850653..e6bc94a008a 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -69,6 +69,8 @@ class RoundRobin : public LoadBalancingPolicy { void HandOffPendingPicksLocked(LoadBalancingPolicy* new_policy) override; void PingOneLocked(grpc_closure* on_initiate, grpc_closure* on_ack) override; void ExitIdleLocked() override; + void FillChildRefsForChannelz(ChildRefsList* child_subchannels, + ChildRefsList* child_channels) override {} private: ~RoundRobin();