From 485c4ba87c2f2cecce7e3104db7db5d50392be2a Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 3 Apr 2018 11:36:23 -0700 Subject: [PATCH] WIP on adding comments. --- .../lb_policy/round_robin/round_robin.cc | 12 ++- .../lb_policy/subchannel_list.h | 87 +++++++++++++++---- 2 files changed, 79 insertions(+), 20 deletions(-) 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 2cfd2d649f7..89d9f670dfb 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 @@ -73,8 +73,15 @@ class RoundRobin : public LoadBalancingPolicy { private: ~RoundRobin(); + // Forward declaration. class RoundRobinSubchannelList; + // Data for a particular subchannel in a subchannel list. + // This subclass adds the following functionality: + // - Tracks user_data associated with each address, which will be + // returned along with picks that select the subchannel. + // - Tracks the previous connectivity state of the subchannel, so that + // we know how many subchannels are in each state. class RoundRobinSubchannelData : public SubchannelData { @@ -91,8 +98,6 @@ class RoundRobin : public LoadBalancingPolicy { ? user_data_vtable_->copy(address.user_data) : nullptr) {} - void ProcessConnectivityChangeLocked(grpc_error* error) override; - void UnrefSubchannelLocked(const char* reason) override { SubchannelData::UnrefSubchannelLocked(reason); if (user_data_ != nullptr) { @@ -105,11 +110,14 @@ class RoundRobin : public LoadBalancingPolicy { void* user_data() const { return user_data_; } private: + void ProcessConnectivityChangeLocked(grpc_error* error) override; + const grpc_lb_user_data_vtable* user_data_vtable_; void* user_data_ = nullptr; grpc_connectivity_state prev_connectivity_state_ = GRPC_CHANNEL_IDLE; }; + // A list of subchannels. class RoundRobinSubchannelList : public SubchannelList { diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index c643f4cb4b4..b647c5d4256 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -38,27 +38,63 @@ #include "src/core/lib/iomgr/sockaddr_utils.h" #include "src/core/lib/transport/connectivity_state.h" -// FIXME: add comments +// Code for maintaining a list of subchannels within an LB policy. +// +// To use this, callers must create their own subclasses, like so: +/* + +class MySubchannelList; // Forward declaration. + +class MySubchannelData + : public SubchannelData { + public: + void ProcessConnectivityChangeLocked(grpc_error* error) override { + // ...code to handle connectivity changes... + } +}; + +class MySubchannelList + : public SubchannelList { +}; + +*/ +// All methods with a Locked() suffix must be called from within the +// client_channel combiner. namespace grpc_core { +// Stores data for a particular subchannel in a subchannel list. +// Callers must create a subclass that implements the +// ProcessConnectivityChangeLocked() method. template class SubchannelData { public: - // Returns the index into subchannel_list_ of this object. + // Returns a pointer to the subchannel list containing this object. + SubchannelListType* subchannel_list() const { return subchannel_list_; } + + // Returns the index into the subchannel list of this object. size_t Index() const { return static_cast(static_cast(this) - subchannel_list_->subchannel(0)); } - SubchannelListType* subchannel_list() const { return subchannel_list_; } - + // Returns a pointer to the subchannel. grpc_subchannel* subchannel() const { return subchannel_; } + // Returns the connected subchannel. Will be null if the subchannel + // is not connected. ConnectedSubchannel* connected_subchannel() const { return connected_subchannel_.get(); } + // The current connectivity state. + // May be called from ProcessConnectivityChangeLocked() to determine + // the state that the subchannel has transitioned into. + grpc_connectivity_state connectivity_state() const { + return curr_connectivity_state_; + } + + // Sets the connected subchannel from the subchannel. void SetConnectedSubchannelFromSubchannelLocked() { connected_subchannel_ = grpc_subchannel_get_connected_subchannel(subchannel_); @@ -75,10 +111,11 @@ class SubchannelData { connected_subchannel_ = other->connected_subchannel_; // Adds ref. } - grpc_connectivity_state connectivity_state() const { - return curr_connectivity_state_; - } - + // Synchronously checks the subchannel's connectivity state. Calls + // ProcessConnectivityChangeLocked() if the state has changed. + // Must not be called while there is a connectivity notification + // pending (i.e., between calling StartConnectivityWatchLocked() and + // the resulting invocation of ProcessConnectivityChangeLocked()). void CheckConnectivityStateLocked() { GPR_ASSERT(!connectivity_notification_pending_); grpc_error* error = GRPC_ERROR_NONE; @@ -90,21 +127,28 @@ class SubchannelData { } } - // Unrefs the subchannel. -// FIXME: move this to private in favor of ShutdownLocked()? + // Unrefs the subchannel. May be used if an individual subchannel is + // no longer needed even though the subchannel list as a whole is not + // being unreffed. virtual void UnrefSubchannelLocked(const char* reason); - /// Starts watching the connectivity state of the subchannel. - /// The connectivity_changed_cb callback must invoke either - /// StopConnectivityWatch() or again call StartConnectivityWatch(). + // Starts or renewes watching the connectivity state of the subchannel. + // ProcessConnectivityChangeLocked() will be called when the + // connectivity state changes. void StartConnectivityWatchLocked(); - /// Stops watching the connectivity state of the subchannel. + // Stops watching the connectivity state of the subchannel. void StopConnectivityWatchLocked(); - /// Cancels watching the connectivity state of the subchannel. + // Cancels watching the connectivity state of the subchannel. + // Must be called only while there is a connectivity notification + // pending (i.e., between calling StartConnectivityWatchLocked() and + // the resulting invocation of ProcessConnectivityChangeLocked()). + // From within ProcessConnectivityChangeLocked(), use + // StopConnectivityWatchLocked() instead. void CancelConnectivityWatchLocked(const char* reason); + // Cancels any pending connectivity watch and unrefs the subchannel. void ShutdownLocked(const char* reason); GRPC_ABSTRACT_BASE_CLASS @@ -118,7 +162,12 @@ class SubchannelData { virtual ~SubchannelData(); -// FIXME: define API + // After StartConnectivityWatchLocked() is called, this method will be + // invoked when the subchannel's connectivity state changes. + // Implementations can use connectivity_state() to get the new + // connectivity state. + // Implementations must invoke either StopConnectivityWatch() or again + // call StartConnectivityWatch() before returning. virtual void ProcessConnectivityChangeLocked(grpc_error* error) GRPC_ABSTRACT; private: @@ -137,13 +186,15 @@ class SubchannelData { bool connectivity_notification_pending_ = false; // Connectivity state to be updated by // grpc_subchannel_notify_on_state_change(), not guarded by - // the combiner. Will be copied to \a curr_connectivity_state_ by - // \a connectivity_changed_closure_. + // the combiner. Will be copied to curr_connectivity_state_ by + // OnConnectivityChangedLocked(). grpc_connectivity_state pending_connectivity_state_unsafe_; // Current connectivity state. grpc_connectivity_state curr_connectivity_state_; }; +// A list of subchannels. +// FIXME: document more template class SubchannelList : public RefCountedWithTracing {