Sorry for the delay! Most of my comments are nit.

But I do have one question:

Decouples connectivity state watching from triggering connection attempts.

What's the benefit of doing this?

Reviewed 3 of 7 files at r1, 5 of 6 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/client_channel.cc, line 1134 at r2 (raw file):

            chand, service_config_json);
  }
  // FIXME: Eliminate this hack as part of hiding health check

Why do we need this hack?

src/core/ext/filters/client_channel/subchannel.h, line 240 at r2 (raw file):

CheckConnectivity

Maybe rename to CheckConnectivityState() to be consistent with WatchConnectivityState().

src/core/ext/filters/client_channel/subchannel.h, line 294 at r2 (raw file):

Subchannel* subchannel,

Since the watcher list is in the subchannel, passing subchannel as a param here doesn't seem very intuitive to me.

Can we pass a connected_subchannel, or storing the subchannel in the watcher list, or document the parameter here?

Or do you intentionally add this to save memory or to simplify object lifetime control?

src/core/ext/filters/client_channel/subchannel.h, line 315 at r2 (raw file):

AddLocked

Maybe change to AddWatcherLocked() to be explicit and consistent with HealthWatcher::AddWatcherLocked()?

src/core/ext/filters/client_channel/subchannel.h, line 319 at r2 (raw file):

                   UniquePtr<char> health_check_service_name,
                   UniquePtr<ConnectivityStateWatcher> watcher);
    void RemoveLocked(const char* health_check_service_name,

Similar here.

src/core/ext/filters/client_channel/subchannel.h, line 385 at r2 (raw file):

  // Connectivity state tracking.
  grpc_connectivity_state state_ = GRPC_CHANNEL_IDLE;
  ConnectivityStateWatcherList watcher_list_;

Please document this member so that it's easier to understand we have both a watcher list and a watcher map.

src/core/ext/filters/client_channel/subchannel.h, line 241 at r3 (raw file):

  // If the return value is GRPC_CHANNEL_READY, also sets *connected_subchannel.
  grpc_connectivity_state CheckConnectivity(
      const char* health_check_service_name,

Please document the parameter health_check_service_name, especially when it's null.

src/core/ext/filters/client_channel/subchannel.cc, line 380 at r2 (raw file):

    UniquePtr<ConnectivityStateWatcher> watcher) {
  watcher->next_ = head_;
  head_ = watcher.release();

Why do we pass watcher as a UniquePtr<> then?

src/core/ext/filters/client_channel/subchannel.cc, line 477 at r2 (raw file):

    if (state_ != GRPC_CHANNEL_CONNECTING) {
      state_ = GRPC_CHANNEL_CONNECTING;
      watcher_list_.NotifyLocked(subchannel_, state_);

Why don't we notify the watchers if the subchannel_state is READY?

Prior to this PR, it looks like we are always reporting CONNECTING to the health tracker.

src/core/ext/filters/client_channel/subchannel.cc, line 561 at r2 (raw file):

  // the map entry.
  if (!it->second->HasWatchers()) {
    map_.erase(it);

This line can be in the line above.

src/core/ext/filters/client_channel/subchannel.cc, line 580 at r2 (raw file):

grpc_connectivity_state Subchannel::HealthWatcherMap::CheckConnectivityLocked(
    Subchannel* subchannel, const char* health_check_service_name) {
  grpc_connectivity_state state;

This method can be

auto it = map_.find(health_check_service_name);
if (it == map_.end()) {
  if (subchannel_->state == GRPC_CHANNEL_READY) return GRPC_CHANNEL_CONNECTING;
  return subchannel_->state;
}
HealthWatcher* health_watcher = it->second.get();
return health_watcher->state();

to be a little shorter.

src/core/ext/filters/client_channel/subchannel.cc, line 932 at r3 (raw file):

                                  GRPC_CHANNEL_TRANSIENT_FAILURE,
                                  "connect_failed");
      c->MaybeStartConnectingLocked();

How will we start reconnecting after this PR?

src/core/ext/filters/client_channel/subchannel.cc, line 954 at r3 (raw file):

  }
  watcher_list_.NotifyLocked(this, state);
  if (state == GRPC_CHANNEL_READY) {

This assumes the caller will only invoke SetConnectivityStateLocked() with a state that's different from the current state.

Please document that.

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h, line 107 at r3 (raw file):

    connectivity_state_ = subchannel()->CheckConnectivity(
        subchannel_list_->health_check_service_name(), &connected_subchannel_);
    return connectivity_state_;

Can return directly in the previous line.

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h, line 442 at r3 (raw file):

void SubchannelData<SubchannelListType, SubchannelDataType>::ShutdownLocked() {
  if (pending_watcher_ != nullptr) {
    CancelConnectivityWatchLocked("shutdown");

I guess this can be appended to the previous line.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 522 at r3 (raw file):

  // watching from this state and will not get a notification of it
  // transitioning into this state.
  // If the current state is not READY, attempt to connect.

We can call AttemptToConnect() without checking because it's a noop if the subchannel is already connected.

Decouples connectivity state watching from triggering connection attempts.

What's the benefit of doing this?

This was necessary to make the PR work. There's basically a cascading set of changes here -- in order to do the thing I really wanted to do here, I had to make another change, which required yet another change, etc.

One of my goals here was to make it so that the subchannel can be shared between channels that have different health check service names. To do that, I needed a way for the subchannel to know the set of health check service names for which it should be maintaining streams to the backend at any given time. I changed it to determine this by getting the health check service name with each caller that is watching the connectivity state. The idea is that it will only maintain a stream to the backend for health check service names for which there is at least one watcher. In other words, when it sees the first watcher for a given health check service name, it will start a stream to the backend for that service name, and when the last watcher for a given health check service name is cancelled, the corresponding stream to the backend will also be cancelled.

However, in order to make that work, I had to change the connectivity watch API. The old API used the renewable callback pattern: the caller called NotifyOnStateChange() to get notified of the next state change, but the callback they provided would only be invoked once, so the caller would have to call NotifyOnStateChange() again if they wanted to renew the watch. The problem with this was that once the subchannel schedules the callback, it has no idea if the caller is going to renew the watch, so it doesn't know whether or not the watch is still ongoing. So I replaced the renewable callback approach with a new API where the caller provides a ConnectivityStateWatcher object that can get multiple callbacks, and that watcher remains present until it is explicitly cancelled by the caller. Because there is no renewal step after each callback, the subchannel can know how many watchers there are for each health check service name at any given time.

That API change is what required decoupling connectivity state watching from triggering connection attempts. Previously, the code would automatically attempt to connect any time there was a pending call to NotifyOnStateChange(), which meant that if we delivered a TRANSIENT_FAILURE notification to the LB policy, we would not start reconnecting until the next call to NotifyOnStateChange(). This allowed policies like pick_first to decide not to renew the watch if they did not want to try to reconnect. However, with the new API, the watcher does not go away and get renewed; it stays present until it is cancelled. So if we didn't decouple these two things, then the subchannel would always automatically start reconnecting after delivering a TRANSIENT_FAILURE notification to the LB policy, because the watch is still there, and it doesn't know whether the LB policy will cancel it. Decoupling these things avoids this problem: when the LB policy gets a TRANSIENT_FAILURE notification, it can either maintain the watch and explicitly ask the subchannel to attempt to reconnect, or it can cancel the watch to stop trying to connect.

It's also worth noting that this change makes the C-core subchannel API a bit more consistent with the subchannel APIs in Java and Go. Both of them have explicit methods for asking the subchannel to attempt to connect, rather than tying it to connectivity state watching.

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @apolcyn and @AspirinSJL)

src/core/ext/filters/client_channel/client_channel.cc, line 1134 at r2 (raw file):

This is actually just restructuring a hack that was added in #18746. It will be removed in #18917, which depends on this PR.

Prior to #18746, the service config (including the health check service name) was a channel arg passed to the subchannel. This meant that we couldn't share subchannels between channels with different health check service names. It also meant that we will needlessly recreate subchannels when a channel gets a service config update that changes its health check service name, because the channel arg is part of the subchannel key.

In #18746, Yash changed the way the service config is handled such that it no longer needs to be passed to the subchannel. However, because this PR wasn't yet ready, the health checking service name still needed to be passed to the subchannel, so Yash added a temporary channel arg to do that. He also changed it such that this parameter was hidden from the LB policy by setting it in ClientChannelControlHelper::CreateSubchannel() instead of doing it here.

In this PR, I am taking a small step backwards by exposing this channel arg to the LB policy instead of hiding it, which is necessary because the new subchannel connectivity state watcher API requires that the LB policy supply the health check service name. I will fix this in #18917 by eliminating the channel arg and completely hiding this parameter behind the SubchannelInterface abstraction. However, I didn't want to add the SubchannelInterface abstraction in this PR, since this is already quite large; I felt it would be easier to review if I split it into two changes.

src/core/ext/filters/client_channel/subchannel.h, line 240 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.h, line 294 at r2 (raw file):

I agree that this is a little messy, but it seemed like the best choice.

I could avoid this by storing a pointer to the subchannel in the ConnectivityStateWatcherList object itself, but then it would unnecessarily increase memory usage.

I could pass in the ConnectedSubchannel instead of the subchannel, but then we would needlessly increment and decrement its refcount in the case where the health watcher is reporting TRANSIENT_FAILURE instead of READY. (Note that ConnectivityStateWatcherList is used in two places: there is a direct member in the Subchannel object for watchers that pass in a null health check service name, and it is also used as a member of HealthWatcherMap::HealthWatcher to store the watchers for a given non-null health check service name.)

Since this is purely an internal API to the subchannel code, I'm willing to live with this slight messiness.

src/core/ext/filters/client_channel/subchannel.h, line 315 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.h, line 319 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.h, line 385 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.h, line 241 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.cc, line 380 at r2 (raw file):

We're making a linked list of watcher objects here, so the code is basically managing the ownership manually. I would have preferred to continue using UniquePtr<> internally, but I didn't see a clean way to make that work without requiring additional memory allocations.

Even though we're manually managing the ownership internally, it's still valuable to use UniquePtr<> in the API used by callers, since it allows the compiler to enforce ownership at least for that part. And it makes it clear to callers what the ownership semantics are of the API.

src/core/ext/filters/client_channel/subchannel.cc, line 477 at r2 (raw file):

The connectivity state notification from the connected subchannel may be lossy: if it transitions quickly from IDLE to CONNECTING to READY, we may get a notification for READY without ever having gotten one for CONNECTING. But we don't want to send duplicate notifications upwards; if we've already notified for CONNECTING, we don't want to do so again here, so we send this only if the state was not already CONNECTING.

I've restructured the code a bit here to try to make the flow a bit clearer.

src/core/ext/filters/client_channel/subchannel.cc, line 561 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.cc, line 580 at r2 (raw file):

Done.

src/core/ext/filters/client_channel/subchannel.cc, line 932 at r3 (raw file):

The LB policy now explicitly calls Subchannel::AttemptToConnect() to request that a connection attempt be started.

src/core/ext/filters/client_channel/subchannel.cc, line 954 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h, line 107 at r3 (raw file):

connectivity_state_ isn't a local variable; it's a class data member that we need to update before returning.

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h, line 442 at r3 (raw file):

Done.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 522 at r3 (raw file):

We could, but it seems cleaner to do it this way, especially since we're already checking the state to see if we need to select the subchannel.

So if we didn't decouple these two things, then the subchannel would always automatically start reconnecting after delivering a TRANSIENT_FAILURE notification to the LB policy, because the watch is still there, and it doesn't know whether the LB policy will cancel it.

Take the pick_first case as an example, the LB policy does cancel the watch once it knows the subchannel is in TRANSIENT_FAILURE. It may start a new watch on that subchannel. When a new watch is started, we can try to connect to that subchannel. So I feel it's OK to couple the connection attempt with starting a watch.

Reviewed 9 of 9 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @markdroth)

src/core/ext/filters/client_channel/subchannel.cc, line 380 at r2 (raw file):

I see.

src/core/ext/filters/client_channel/subchannel.cc, line 477 at r2 (raw file):

My precious comment is not clear.

What I wanted to say is: if the subchannel_state passed to ctor is READY, we degrade it to CONNECTING; when StartHealthCheckingLocked() is called, we don't notify the watchers about the state CONNECTING.

But prior to this PR, we will notify the watchers in this case.

The problem is that when we transition into TRANSIENT_FAILURE, the subchannel code would already have started trying to reconnect by the time the LB policy cancels the watch.

In any case, I would argue that decoupling these two things actually makes for a clearer API anyway. It's really fairly counter-intuitive that starting a connectivity state watch would also trigger the start of a connection attempt, which is why neither Java nor Go do it that way.

In addition, coupling these two things will also cause problems in the future when we move the backoff code out of the subchannel, because while we're in backoff state, we may still want to continue watching the subchannels' states in case some other channel using the same subchannel causes it to reconnect.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @AspirinSJL)

src/core/ext/filters/client_channel/subchannel.cc, line 477 at r2 (raw file):

When the ctor is called, there are no watchers yet; the list starts out empty. When watchers are subsequently added via AddWatcherLocked(), we immediately notify them if the current state is not their initial state.

The problem is that when we transition into TRANSIENT_FAILURE, the subchannel code would already have started trying to reconnect by the time the LB policy cancels the watch.

We can remove the immediate reconnection in Subchannel::OnConnectivityChanged() and Subchannel::OnConnectingFinished(), just like this PR has done.

In addition, coupling these two things will also cause problems in the future when we move the backoff code out of the subchannel, because while we're in backoff state, we may still want to continue watching the subchannels' states in case some other channel using the same subchannel causes it to reconnect.

I agree this is a strong argument.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @apolcyn)

src/core/ext/filters/client_channel/subchannel.cc, line 477 at r2 (raw file):

I see.