Reviewed 2 of 23 files at r1, 17 of 22 files at r2.
Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @apolcyn, @AspirinSJL, and @markdroth)

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

 private:
  class WatcherWrapper : public Subchannel::ConnectivityStateWatcher {

Could you describe the relationship between the various kinds of watcher classes a little more? I'm kind of lost here.

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

  Subchannel* subchannel_;
  UniquePtr<char> health_check_service_name_;
  // Maps from the address of the wrapper watcher passed to us to the

The map is actually from ConnectivityStateWatcher to WatcherWrapper?

Reviewed 1 of 22 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn and @markdroth)

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

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

I'm glad you asked about this. I was going to add some comments explaining this, but in the process of doing that, I realized that this level of indirection was not actually necessary. I've made some changes to remove the indirection, so the WrapperWatcher class has been removed.

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

You're right, the comment was backwards. But this map has been removed now.

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

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

// TODO(roth): Should not need the include of subchannel.h here, since
// that implementation should be hidden from the LB policy API.
#include "src/core/ext/filters/client_channel/subchannel.h"

Why do we add it back then?

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

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

It's needed for the subchannel address channel arg. It was previously being included transitively, because it was included in subchannel_interface.h, but I removed that include, since subchannel.h now needs to include subchannel_interface.h for the ConnectivityStateWatcher definition.

I will clean this up in a later PR.

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

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

I see. Thanks for explaining!

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