Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17866 )
Change subject: [rpc] Expose KRPC call_id to client application ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1589 PS1, Line 1589: // Set up client. : LOG(INFO) << "Connecting to " << server_addr.ToString(); Consider dropping this once you are done with developing this test scenario: out tests are too chatty already and this isn't not going to be used by the automated testing framework anyways. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1595 PS1, Line 1595: ASSERT_STR_CONTAINS(p.ToString(), Substitute("kudu.rpc.GenericCalculatorService@" : "{remote=$0, user_credentials=", : expected_remote_str(server_addr))); Is this assert essential for this scenario? If yes, please add a comment explaining why, otherwise maybe drop this? http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1603 PS1, Line 1603: AddResponsePB resp; nit: move this closer to the place where 'resp' is used. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1606 PS1, Line 1606: controller.set_credentials_policy(CredentialsPolicy::ANY_CREDENTIALS); That's the default setting for credentials policy: consider dropping this line. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1607 PS1, Line 1607: CHECK_OK Here and below: replace CHECK_XXX() with ASSERT_XXX(): failing the test with a coredump due to SIGABRT isn't a very good choice compared with regular reporting used by ASSERT_XXX(). http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1611 PS1, Line 1611: CHECK_EQ(controller.call_id(), i) Once replaced with ASSERT_EQ(), swap the order of the arguments -- the expected/reference value comes first (that way it's easier to read the failure report if assert is ever triggered). http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1611 PS1, Line 1611: CHECK_EQ(controller.call_id(), i); > AFAIK, call_id is monotonically increasing per host across all RPC That's per RPC connection, if I'm not mistaken -- i.e. if another connection is established, the sequence restarts. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.h@230 PS1, Line 230: const nit: there isn't much sense in returning a constant by value http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc File src/kudu/rpc/rpc_context.cc: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc@147 PS1, Line 147: call_->header().call_id() Might it be just call->call_id() ? http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.h@213 PS1, Line 213: call's finished nit: could you extend this comment to explain what "call's finished" means and how to detect that? Maybe mention that InboundCall::ParseFrom() should be called to have call id initialized (alternatively, maybe mention that Connection::HandleIncomingCall() should have been called for this call)? http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc File src/kudu/rpc/rpc_controller.cc: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc@106 PS1, Line 106: const nit: returning constant primitive types by value doesn't make much sense -- consider dropping 'const' -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 24 Sep 2021 00:00:27 +0000 Gerrit-HasComments: Yes
