Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17866 )
Change subject: [rpc] Expose KRPC call_id to client application ...................................................................... Patch Set 3: (12 comments) Thank you for the feedback! 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. : shared_ptr<Messenger> client_messenger; > Consider dropping this once you are done with developing this test scenario Dropped. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1595 PS1, Line 1595: for (int i = 0; i < 10; i++) { : AddRequestPB req; : req.set_x(rand()); > Is this assert essential for this scenario? If yes, please add a comment e Dropped. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1603 PS1, Line 1603: ASSERT_OK(p.SyncReq > nit: move this closer to the place where 'resp' is used. Done http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1606 PS1, Line 1606: ASSERT_EQ(i, controller.call_id()); > That's the default setting for credentials policy: consider dropping this l Dropped. http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1607 PS1, Line 1607: > Here and below: replace CHECK_XXX() with ASSERT_XXX(): failing the test wit Done http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1611 PS1, Line 1611: namespace kudu > Once replaced with ASSERT_EQ(), swap the order of the arguments -- the expe Done 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: > nit: there isn't much sense in returning a constant by value Done 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@146 PS1, Line 146: int32_t RpcContext::call_id() const { > warning: return type 'const int32_t' (aka 'const int') is 'const'-qualified Done http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc@147 PS1, Line 147: call_->call_id(); > Might it be just call->call_id() ? Done 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: he call's Respo > nit: could you extend this comment to explain what "call's finished" means Since RpcController reside in the client, "call's finished" means the Response has been received. Updated the comment accordingly. 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: int32_t RpcController::call_id() const { > warning: return type 'const int32_t' (aka 'const int') is 'const'-qualified Done http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc@106 PS1, Line 106: int32 > nit: returning constant primitive types by value doesn't make much sense -- Done -- 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: 3 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 01:27:14 +0000 Gerrit-HasComments: Yes
