Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )
Change subject: Add "network_plane" as part of ConnectionId ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7 PS1, Line 7: Add "service_name" as part of ConnectionId > I'm curious to hear your thinking on, when implementing option 1, you did i Specifying it per service seems a bit rigid as it hardcodes the data plane for all calls in the entire service. Specifying it per-RPC is vs per-Proxy can be functionally equivalent. After all, a proxy is needed to make a RPC call. One can have multiple proxies with different data planes and pick the right one for a given call. It's also more convenient as the ConnectionId is owned by the Proxy (as the code stands now). Piping the "network plane" through per-RPC seems slightly more complicated. 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: } > Nit: we're using std::string here; don't need std:: prefix. Done http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62 PS4, Line 62: user_credentials_.ToString()); > Maybe omit the plane altogether if it's the default one? Done 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: r after a > Nit: "so that a higher" Done http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55 PS4, Line 55: arted will caus > Nit: "a control channel" Done http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118 PS4, Line 118: mutable Atomic32 is_started_; > Could we pass by copy here and std::move into the underlying ConnectionId? Done; Did the same for set_user_credentials(). 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: req, &resp, &controller)); > There's a mix of ASSERT and EXPECT below; curious why you aren't just using Just copied and pasted that pattern from TestCredentialsPolicy. I don't see any reason for such mixed use. Converted everything to ASSERT instead. http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697 PS4, Line 697: } > Test num_{client,server}_connections here too? Done http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698 PS4, Line 698: : // Test that the RpcSidecar transfers the expected messages. : TEST_P(TestRpc, TestRpcSidecar) { : // Set up server. : Sockaddr server_addr; : bool enable_ssl = GetParam(); : ASSERT_OK(StartTestServer(&server_addr, enable_s > This is repeated below; perhaps define a 2-arg lambda to encapsulate the wh These verification logic seem to be repeated across multiple tests at various places. I agree that we can encapsulate these verification logic into a function or lambda. However, the downside seems to be that it may be harder to identify the failures. As the code stands now, the failure can be easily identified by line number. -- 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: 1 Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 08 Nov 2018 00:22:53 +0000 Gerrit-HasComments: Yes
