Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: PS2, Line 60: inline Status FromKuduStatus Leave a TODO if we're planning on standardizing the Status class. http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-builder.h File be/src/rpc/rpc-builder.h: PS2, Line 109: if (!err || !err->has_code() : || err->code() != kudu::rpc::ErrorStatusPB::ERROR_SERVER_TOO_BUSY) { : return FromKuduStatus(controller.status()); : } Should we add a comment stating what sort of error/error codes don't warrant a retry? Or something like "Retry RPC if the server is too busy, else return success or error status." PS2, Line 131: DeserializeThriftFromProtoWrapper(response_proto, resp); Check return val? PS2, Line 157: MakeRpc This function name sounds like it's executing the RPC, akin to ExecuteRPC(). Would something like MakeRpcObject be a more suitable name? http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: PS2, Line 143: ServiceRegistration Do you think this struct is really very necessary? We can do the service registration with the Messenger in RpcMgr::RegisterService() and just save the service_pool objects. We can then call Init() on these service_pool objects in StartServices(). If you were thinking this would be useful information to add to the debug webpages, we can get the same info from the ServicePool object itself. Is there any other use you foresee for this object? http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: PS2, Line 101: SerializeThriftToProtoWrapper(&thrift_response, response); Check the return value? PS2, Line 114: SerializeThriftToProtoWrapper(&thrift_response, response); Same here. http://gerrit.cloudera.org:8080/#/c/5720/2/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: PS2, Line 772: 32, 1024 Should we make these const uints for clarity? -- To view, visit http://gerrit.cloudera.org:8080/5720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8dbf12b9ecd71d26d239d31c19b487175194c766 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
