Reviewed 20 of 20 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @markdroth)

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

      GRPC_ERROR_REF(result.service_config_error);
  if (service_config == nullptr &&
      result.service_config_error != GRPC_ERROR_NONE) {

because of this extra check "result.service_config_error != GRPC_ERROR_NONE" we cannot assume that code after will be the case "service_config != nullptr"; does it make sense to deal with that specific case and then for everything else you will not have to have many of the check for "if (service_config != nullptr) "

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

  // Check if the config has changed.
  service_config_result.service_config_changed =
      ((service_config == nullptr) !=

Here is the logic one must be null while the other must be not-null? just checking

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @donnadionne)

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

I'm not sure exactly what you're suggesting here. We need to check these two conditions independently, because they can be set independently. Even if the resolver returns a config error, we might have decided to use the default config or a saved config above.

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

chand_->saved_service_config_ is the config we used after the previous resolver result, and service_config is the one we are going to use after this new resolver result. If the old config did not exist and the new one does, or if the old config did exist and the new one does not, then the config has changed.

Note that this logic didn't really change as part of this PR; it just moved here from ChannelData::ProcessResolverResultLocked()