Reviewed 2 of 2 files at r3, 3 of 8 files at r4, 9 of 9 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @apolcyn, @markdroth, and @mhaidrygoog)

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

ConnectivityStateWatcher

We have three layers of classes now:

1. SubchannelInterface::ConnectivityStateWatcher
2. Subchannel::ConnectivityStateWatcher
3. SubchannelWrapper::WatcherWrapper

To be consistent and clear, maybe we should change 1 to SubchannelInterface::ConnectivityStateWatcherInterface.

I'm also fine with a verbose SubchannelWrapper::ConnectivityStateWatcherWrapper.

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

CallCombiner

It's not call combiner?

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

        self->parent_->parent_->MaybeUpdateConnectedSubchannel(
            std::move(self->connected_subchannel_));
        self->parent_->watcher_->OnConnectivityStateChange(self->state_);

This doesn't need to be done in the combiner?

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

}

RefCountedPtr<ConnectedSubchannel> ChannelData::GetConnectedSubchannel(

Please indicate that we are returning the one in data plane, either in the method name or a comment.

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

grpc_subchannel

subchannel_wrapper

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

pick

The previous name args might be more accurate.

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

  }
  // Cases 1 and 2.
  if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_pick_first_trace)) {

We can keep this log after the update.

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

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

I agree that the nomenclature here is a bit confusing, but I'm not sure that either of your suggestions actually makes it clearer.

Note that what we actually have here is two interfaces and two concrete implementations. The interfaces are:

• Subchannel::ConnectivityStateWatcher: The "real" watcher API for the "real" subchannel. Requires the caller to specify the health check service name and returns the connected subchannel.
• SubchannelInterface::ConnectivityStateWatcher: The watcher API exposed to LB policies. Does not know anything about health checking or connected subchannels.

The concrete implementations are:

• SubchannelData<>::Watcher: Implements SubchannelInterface::ConnectivityStateWatcher. This is effectively part of the LB policy implementation, so it uses the SubchannelInterface API instead of the "real" subchannel API.
• SubchannelWrapper::WatcherWrapper: Implements Subchannel::ConnectivityStateWatcher. This gets updates from the "real" subchannel and passes them to the watcher given to us by the LB policy.

Changing SubchannelInterface::ConnectivityStateWatcher to SubchannelInterface::ConnectivityStateWatcherInterface doesn't seem to help much, since (a) that class is already nested in an interface class, so it should already be clear that it's an interface, and (b) it wouldn't really differentiate it from Subchannel::ConnectivityStateWatcher, since that's also an interface.

Changing SubchannelWrapper::WatcherWrapper to SubchannelWrapper::ConnectivityStateWatcherWrapper also doesn't seem to help much, since there's really no other type of watcher here other than a connectivity state watcher, so it's not clear what else could be wrapped here.

I would welcome alternative suggestions here. It might also help if you could explain exactly what type of confusion you're looking to avoid -- e.g., which of the existing classes are too easily confused with each other.

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

Good catch. No idea why I typed that -- this is actually running in the channel's control plane combiner, not the call combiner.

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

Yes, it does, and it is.

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

Done.

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

Done.

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

Done.

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

I think it actually makes more sense before the update, because this way we see why pick_first made this decision before we see the log where the update is processed by the channel.

Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @markdroth, and @mhaidrygoog)

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

Thanks for the explanation! This makes more sense now.

I think it's important to notice that SubchannelWrapper::WatcherWrapper implements Subchannel::ConnectivityStateWatcher and wraps SubchannelInterface::ConnectivityStateWatcher, which is implemented by SubchannelData<>::Watcher.

When I first read the code, it's not clear to me which watcher is interface and which watcher is implementation. It's not common that we have two interfaces with the same name? Now that I can understand the relationship between these four classes, I can see that ConnectivityStateWatchers are interfaces. I think it's clearer to explicitly say that both of them are interfaces because otherwise it's not intuitive that Subchannel::ConnectivityStateWatcher is also an interface. Maybe just call them WatcherInterface?

And we'd better document SubchannelWrapper::WatcherWrapper in more details, like, it wraps SubchannelInterface::ConnectivityStateWatcher, because SubchannelWrapper::WatcherWrapper's role is the most complex among the four.

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

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

Okay, I've added the Interface suffix to both ConnectivityStateWatcher classes, and I've expanded the comments in various places to help make the relationships clearer.

Please let me know if anything is still unclear.

Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @mhaidrygoog)