Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4670 / IMPALA-4672: Add RpcMgr and port Statestore services to KRPC ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: PS3, Line 52: 4 Could be left as future work, but just wanted to bring it up. Do you think this value should be passed as an argument into Init(), since we may identify that we need a different number of reactor threads for different daemon types? PS3, Line 64: service_pool->Init Safer to Init() after calling messenger_->RegisterService() ? PS3, Line 74: int32_t num_acceptor_threads If this is already a user controlled flag, why pass it in as a parameter? http://gerrit.cloudera.org:8080/#/c/5720/3/be/src/rpc/rpc.h File be/src/rpc/rpc.h: PS3, Line 90: func As I understand it currently, if 'func' is the asynchronous variant of the remote method, we could end up with some sort of runtime error in L108, as we're not calling it with a callback argument. I don't know what's the best way to do this, but should we disallow calling asynchronous functions with Execute() somehow? And introduce an ExecuteAsync() variant? -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Juan Yu <j...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
