impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC
Date Wed, 18 Jan 2017 23:44:25 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message