Reviewer comments

src/core/ext/filters/client_channel/client_channel.cc

@@ -273,7 +273,7 @@ class ChannelData {
   ExternalConnectivityWatcher::WatcherList external_connectivity_watcher_list_;
   UniquePtr<char> health_check_service_name_;
   RefCountedPtr<ServiceConfig> saved_service_config_;
-  bool set_service_config_ = false;
+  bool received_first_resolver_result_ = false;
 
   //
   // Fields accessed from both data plane and control plane combiners.
@@ -1249,7 +1249,7 @@ bool ChannelData::ProcessResolverResultLocked(
       result.service_config_error != GRPC_ERROR_NONE) {
     return false;
   }
-  UniquePtr<char> service_config_json = nullptr;
+  UniquePtr<char> service_config_json;
   // Process service config.
   const internal::ClientChannelGlobalParsedObject* parsed_object = nullptr;
   if (service_config != nullptr) {
@@ -1265,14 +1265,12 @@ bool ChannelData::ProcessResolverResultLocked(
        strcmp(service_config->service_config_json(),
               chand->saved_service_config_->service_config_json()) != 0);
   if (service_config_changed) {
-    if (service_config != nullptr) {
-      service_config_json.reset(
-          gpr_strdup(service_config->service_config_json()));
-    } else {
-      service_config_json.reset(gpr_strdup(""));
-    }
+    service_config_json.reset(gpr_strdup(
+        service_config != nullptr ? service_config->service_config_json()
+                                  : ""));
     if (grpc_client_channel_routing_trace.enabled()) {
-      gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"",
+      gpr_log(GPR_INFO,
+              "chand=%p: resolver returned updated service config: \"%s\"",
               chand, service_config_json.get());
     }
     chand->saved_service_config_ = std::move(service_config);
@@ -1283,8 +1281,11 @@ bool ChannelData::ProcessResolverResultLocked(
       chand->health_check_service_name_.reset();
     }
   }
-  if (service_config_changed || !chand->set_service_config_) {
-    chand->set_service_config_ = true;
+  // We want to set the service config atleast once. This should not really be
+  // needed, but we are doing it as a defensive approach. This can be removed,
+  // if we feel it is unnecessary.
+  if (service_config_changed || !chand->received_first_resolver_result_) {
+    chand->received_first_resolver_result_ = true;
     Optional<internal::ClientChannelGlobalParsedObject::RetryThrottling>
         retry_throttle_data;
     if (parsed_object != nullptr) {
@@ -3029,6 +3030,8 @@ void CallData::AddSubchannelBatchesForPendingBatches(
     // If we're not retrying, just send the batch as-is.
     if (method_params_ == nullptr ||
         method_params_->retry_policy() == nullptr || retry_committed_) {
+      // TODO(roth) : We should probably call
+      // MaybeInjectRecvTrailingMetadataReadyForLoadBalancingPolicy here.
       AddClosureForSubchannelBatch(elem, batch, closures);
       PendingBatchClear(pending);
       continue;

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h

@@ -506,6 +506,10 @@ SubchannelList<SubchannelListType, SubchannelDataType>::SubchannelList(
                                          GRPC_ARG_INHIBIT_HEALTH_CHECKING};
   // Create a subchannel for each address.
   for (size_t i = 0; i < addresses.size(); i++) {
+    // TODO(roth): we should ideally hide this from the LB policy code. In
+    // principle, if we're dealing with this special case in the client_channel
+    // code for selecting grpclb, then we should also strip out these addresses
+    // there if we're not using grpclb.
     if (addresses[i].IsBalancer()) {
       continue;
     }

test/cpp/end2end/grpclb_end2end_test.cc

@@ -738,7 +738,7 @@ TEST_F(SingleBalancerTest, SelectGrpclbWithMigrationServiceConfig) {
 }
 
 TEST_F(SingleBalancerTest,
-       DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest1) {
+       DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest) {
   const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor();
   ResetStub(kFallbackTimeoutMs);
   SetNextResolution({AddressData{backends_[0]->port_, false, ""},
@@ -753,8 +753,9 @@ TEST_F(SingleBalancerTest,
   EXPECT_EQ("pick_first", channel_->GetLoadBalancingPolicyName());
 }
 
-TEST_F(SingleBalancerTest,
-       DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest2) {
+TEST_F(
+    SingleBalancerTest,
+    DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTestAndNoBackendAddress) {
   const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor();
   ResetStub(kFallbackTimeoutMs);
   SetNextResolution({AddressData{balancers_[0]->port_, true, ""}},