Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )
Change subject: Add "network_plane" as part of ConnectionId ...................................................................... Patch Set 4: (8 comments) Could you also update this comment in reactor.h: // Client-side connection map. Multiple connections could be open to a remote // server if multiple credential policies are used for individual RPCs. While true, the wording should be made more generic since a connection ID is now more than just a remote and set of credentials. http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc File src/kudu/rpc/connection_id.cc: http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@50 PS4, Line 50: void ConnectionId::set_network_plane(std::string network_plane) { Nit: we're using std::string here; don't need std:: prefix. http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62 PS4, Line 62: return strings::Substitute("{remote=$0, user_credentials=$1, network_plane=$2}", Maybe omit the plane altogether if it's the default one? http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h File src/kudu/rpc/proxy.h: http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@54 PS4, Line 54: so higher Nit: "so that a higher" http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55 PS4, Line 55: control channel Nit: "a control channel" http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118 PS4, Line 118: void set_network_plane(const std::string& network_plane); Could we pass by copy here and std::move into the underlying ConnectionId? http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696 PS4, Line 696: There's a mix of ASSERT and EXPECT below; curious why you aren't just using one and not the other? http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697 PS4, Line 697: // Verify the initial counters. Test num_{client,server}_connections here too? http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698 PS4, Line 698: ReactorMetrics metrics; : ASSERT_OK(server_messenger_->reactors_[0]->GetMetrics(&metrics)); : ASSERT_EQ(0, metrics.total_client_connections_); : ASSERT_EQ(0, metrics.total_server_connections_); : ASSERT_OK(client_messenger->reactors_[0]->GetMetrics(&metrics)); : ASSERT_EQ(0, metrics.total_client_connections_); : ASSERT_EQ(0, metrics.total_server_connections_); This is repeated below; perhaps define a 2-arg lambda to encapsulate the whole thing? -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 07 Nov 2018 00:06:18 +0000 Gerrit-HasComments: Yes
