Fixing code review comments.

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

@@ -1046,6 +1046,10 @@ grpc_error* RouteConfigParse(
       }
       rds_route.service = std::string(path_elements[0]);
       rds_route.method = std::string(path_elements[1]);
+    } else {
+      // TODO(donnadionne): We may change this behavior once we decide how to
+      // handle unsupported fields.
+      continue;
     }
     if (!envoy_api_v2_route_Route_has_route(route)) {
       return GRPC_ERROR_CREATE_FROM_STATIC_STRING(

test/cpp/end2end/test_service_impl.h

@@ -57,12 +57,47 @@ typedef enum {
   CANCEL_AFTER_PROCESSING
 } ServerTryCancelRequestPhase;
 
-namespace {
+class TestServiceSignaller {
+ public:
+  void ClientWaitUntilRpcStarted() {
+    std::unique_lock<std::mutex> lock(mu_);
+    cv_rpc_started_.wait(lock, [this] { return rpc_started_; });
+  }
+  void ServerWaitToContinue() {
+    std::unique_lock<std::mutex> lock(mu_);
+    cv_server_continue_.wait(lock, [this] { return server_should_continue_; });
+  }
+  void SignalClientThatRpcStarted() {
+    std::unique_lock<std::mutex> lock(mu_);
+    rpc_started_ = true;
+    cv_rpc_started_.notify_one();
+  }
+  void SignalServerToContinue() {
+    std::unique_lock<std::mutex> lock(mu_);
+    server_should_continue_ = true;
+    cv_server_continue_.notify_one();
+  }
 
-// When echo_deadline is requested, deadline seen in the ServerContext is set in
-// the response in seconds.
-void MaybeEchoDeadline(experimental::ServerContextBase* context,
-                       const EchoRequest* request, EchoResponse* response) {
+ private:
+  std::mutex mu_;
+  std::condition_variable cv_rpc_started_;
+  bool rpc_started_ /* GUARDED_BY(mu_) */ = false;
+  std::condition_variable cv_server_continue_;
+  bool server_should_continue_ /* GUARDED_BY(mu_) */ = false;
+};
+
+template <typename RpcService>
+class TestMultipleServiceImpl : public RpcService {
+ public:
+  TestMultipleServiceImpl() : signal_client_(false), host_() {}
+  explicit TestMultipleServiceImpl(const grpc::string& host)
+      : signal_client_(false), host_(new grpc::string(host)) {}
+
+  // When echo_deadline is requested, deadline seen in the ServerContext is set
+  // in the response in seconds.
+  void static MaybeEchoDeadline(experimental::ServerContextBase* context,
+                                const EchoRequest* request,
+                                EchoResponse* response) {
     if (request->has_param() && request->param().echo_deadline()) {
       gpr_timespec deadline = gpr_inf_future(GPR_CLOCK_REALTIME);
       if (context->deadline() != system_clock::time_point::max()) {
@@ -70,9 +105,9 @@ void MaybeEchoDeadline(experimental::ServerContextBase* context,
       }
       response->mutable_param()->set_request_deadline(deadline.tv_sec);
     }
-}
+  }
 
-void CheckServerAuthContext(
+  void static CheckServerAuthContext(
       const experimental::ServerContextBase* context,
       const grpc::string& expected_transport_security_type,
       const grpc::string& expected_client_identity) {
@@ -91,11 +126,11 @@ void CheckServerAuthContext(
       EXPECT_EQ(1u, identity.size());
       EXPECT_EQ(expected_client_identity, identity[0]);
     }
-}
+  }
 
-// Returns the number of pairs in metadata that exactly match the given
-// key-value pair. Returns -1 if the pair wasn't found.
-int MetadataMatchCount(
+  // Returns the number of pairs in metadata that exactly match the given
+  // key-value pair. Returns -1 if the pair wasn't found.
+  int static MetadataMatchCount(
       const std::multimap<grpc::string_ref, grpc::string_ref>& metadata,
       const grpc::string& key, const grpc::string& value) {
     int count = 0;
@@ -106,11 +141,9 @@ int MetadataMatchCount(
       }
     }
     return count;
-}
-}  // namespace
+  }
 
-namespace {
-int GetIntValueFromMetadataHelper(
+  int static GetIntValueFromMetadataHelper(
       const char* key,
       const std::multimap<grpc::string_ref, grpc::string_ref>& metadata,
       int default_value) {
@@ -121,16 +154,16 @@ int GetIntValueFromMetadataHelper(
     }
 
     return default_value;
-}
+  }
 
-int GetIntValueFromMetadata(
+  int static GetIntValueFromMetadata(
       const char* key,
       const std::multimap<grpc::string_ref, grpc::string_ref>& metadata,
       int default_value) {
     return GetIntValueFromMetadataHelper(key, metadata, default_value);
-}
+  }
 
-void ServerTryCancel(ServerContext* context) {
+  void static ServerTryCancel(ServerContext* context) {
     EXPECT_FALSE(context->IsCancelled());
     context->TryCancel();
     gpr_log(GPR_INFO, "Server called TryCancel() to cancel the request");
@@ -139,46 +172,8 @@ void ServerTryCancel(ServerContext* context) {
       gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME),
                                    gpr_time_from_micros(1000, GPR_TIMESPAN)));
     }
-}
-
-}  // namespace
-
-class TestServiceSignaller {
- public:
-  void ClientWaitUntilRpcStarted() {
-    std::unique_lock<std::mutex> lock(mu_);
-    cv_rpc_started_.wait(lock, [this] { return rpc_started_; });
-  }
-  void ServerWaitToContinue() {
-    std::unique_lock<std::mutex> lock(mu_);
-    cv_server_continue_.wait(lock, [this] { return server_should_continue_; });
-  }
-  void SignalClientThatRpcStarted() {
-    std::unique_lock<std::mutex> lock(mu_);
-    rpc_started_ = true;
-    cv_rpc_started_.notify_one();
-  }
-  void SignalServerToContinue() {
-    std::unique_lock<std::mutex> lock(mu_);
-    server_should_continue_ = true;
-    cv_server_continue_.notify_one();
   }
 
- private:
-  std::mutex mu_;
-  std::condition_variable cv_rpc_started_;
-  bool rpc_started_ /* GUARDED_BY(mu_) */ = false;
-  std::condition_variable cv_server_continue_;
-  bool server_should_continue_ /* GUARDED_BY(mu_) */ = false;
-};
-
-template <typename RpcService>
-class TestMultipleServiceImpl : public RpcService {
- public:
-  TestMultipleServiceImpl() : signal_client_(false), host_() {}
-  explicit TestMultipleServiceImpl(const grpc::string& host)
-      : signal_client_(false), host_(new grpc::string(host)) {}
-
   Status Echo(ServerContext* context, const EchoRequest* request,
               EchoResponse* response) {
     if (request->has_param() &&
@@ -311,6 +306,7 @@ class TestMultipleServiceImpl : public RpcService {
   }
 
   // Unimplemented is left unimplemented to test the returned error.
+
   Status RequestStream(ServerContext* context,
                        ServerReader<EchoRequest>* reader,
                        EchoResponse* response) {

test/cpp/end2end/xds_end2end_test.cc

@@ -264,12 +264,12 @@ class BackendServiceImpl
 
   Status Echo1(ServerContext* context, const EchoRequest* request,
                EchoResponse* response) override {
-    return (Echo(context, request, response));
+    return Echo(context, request, response);
   }
 
   Status Echo2(ServerContext* context, const EchoRequest* request,
                EchoResponse* response) override {
-    return (Echo(context, request, response));
+    return Echo(context, request, response);
   }
 
   void Start() {}
@@ -2197,10 +2197,10 @@ TEST_P(LdsTest, RouteMatchHasNonemptyPrefix) {
 TEST_P(LdsTest, RouteMatchHasEmptyPath) {
   RouteConfiguration route_config =
       balancers_[0]->ads_service()->default_route_config();
-  route_config.mutable_virtual_hosts(0)
-      ->mutable_routes(0)
-      ->mutable_match()
-      ->set_path("");
+  auto* route1 = route_config.mutable_virtual_hosts(0)->mutable_routes(0);
+  route1->mutable_match()->set_path("");
+  auto* default_route = route_config.mutable_virtual_hosts(0)->add_routes();
+  default_route->mutable_match()->set_prefix("");
   balancers_[0]->ads_service()->SetLdsResource(
       AdsServiceImpl::BuildListener(route_config), kDefaultResourceName);
   SetNextResolution({});
@@ -2291,20 +2291,15 @@ TEST_P(LdsTest, XdsRoutingPathMatching) {
   Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster();
   new_cluster2.set_name(kNewCluster2Name);
   balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name);
-  // Change RDS resource to set up prefix matching to direct traffic to the
-  // second new cluster.
+  // Populating Route Configurations for LDS.
   RouteConfiguration new_route_config =
       balancers_[0]->ads_service()->default_route_config();
-  auto* mismatched_route1 =
-      new_route_config.mutable_virtual_hosts(0)->mutable_routes(0);
-  mismatched_route1->mutable_match()->set_path(
-      "/grpc.testing.EchoTest1Service/Echo1");
-  mismatched_route1->mutable_route()->set_cluster(kNewCluster1Name);
-  auto* mismatched_route2 =
-      new_route_config.mutable_virtual_hosts(0)->add_routes();
-  mismatched_route2->mutable_match()->set_path(
-      "/grpc.testing.EchoTest2Service/Echo2");
-  mismatched_route2->mutable_route()->set_cluster(kNewCluster2Name);
+  auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0);
+  route1->mutable_match()->set_path("/grpc.testing.EchoTest1Service/Echo1");
+  route1->mutable_route()->set_cluster(kNewCluster1Name);
+  auto* route2 = new_route_config.mutable_virtual_hosts(0)->add_routes();
+  route2->mutable_match()->set_path("/grpc.testing.EchoTest2Service/Echo2");
+  route2->mutable_route()->set_cluster(kNewCluster2Name);
   auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
   default_route->mutable_match()->set_prefix("");
   default_route->mutable_route()->set_cluster(kDefaultResourceName);
@@ -2367,18 +2362,15 @@ TEST_P(LdsTest, XdsRoutingPrefixMatching) {
   Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster();
   new_cluster2.set_name(kNewCluster2Name);
   balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name);
-  // Change RDS resource to set up prefix matching to direct traffic to the
-  // second new cluster.
+  // Populating Route Configurations for LDS.
   RouteConfiguration new_route_config =
       balancers_[0]->ads_service()->default_route_config();
-  auto* mismatched_route =
-      new_route_config.mutable_virtual_hosts(0)->mutable_routes(0);
-  mismatched_route->mutable_match()->set_prefix(
-      "/grpc.testing.EchoTest1Service");
-  mismatched_route->mutable_route()->set_cluster(kNewCluster1Name);
-  auto* matched_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
-  matched_route->mutable_match()->set_prefix("/grpc.testing.EchoTest2Service");
-  matched_route->mutable_route()->set_cluster(kNewCluster2Name);
+  auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0);
+  route1->mutable_match()->set_prefix("/grpc.testing.EchoTest1Service");
+  route1->mutable_route()->set_cluster(kNewCluster1Name);
+  auto* route2 = new_route_config.mutable_virtual_hosts(0)->add_routes();
+  route2->mutable_match()->set_prefix("/grpc.testing.EchoTest2Service");
+  route2->mutable_route()->set_cluster(kNewCluster2Name);
   auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes();
   default_route->mutable_match()->set_prefix("");
   default_route->mutable_route()->set_cluster(kDefaultResourceName);

test/cpp/util/grpc_tool_test.cc

@@ -1107,9 +1107,10 @@ TEST_F(GrpcToolTest, CallCommandWithMetadata) {
   ShutdownServer();
 }
 
-/*TEST_F(GrpcToolTest, CallCommandWithBadMetadata) {
+TEST_F(GrpcToolTest, CallCommandWithBadMetadata) {
   // Test input "grpc_cli call localhost:10000 Echo "message: 'Hello'"
-  const char* argv[] = {"grpc_cli", "call", "localhost:10000", "Echo",
+  const char* argv[] = {"grpc_cli", "call", "localhost:10000",
+                        "grpc.testing.EchoTestService.Echo",
                         "message: 'Hello'"};
   FLAGS_protofiles = "src/proto/grpc/testing/echo.proto";
   char* test_srcdir = gpr_getenv("TEST_SRCDIR");
@@ -1143,7 +1144,7 @@ TEST_F(GrpcToolTest, CallCommandWithMetadata) {
   FLAGS_protofiles = "";
 
   gpr_free(test_srcdir);
-}*/
+}
 
 TEST_F(GrpcToolTest, ListCommand_OverrideSslHostName) {
   const grpc::string server_address = SetUpServer(true);