Henry Robinson 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 Yeah, this should be plumbed through, will do that. PS3, Line 64: service_pool->Init > Safer to Init() after calling messenger_->RegisterService() ? Hm, why do you think so? Seems more safe to get the threads running before the service is 'exposed' via RegisterService() (even though StartServices() is where requests are actually allowed to start arriving). PS3, Line 74: int32_t num_acceptor_threads > If this is already a user controlled flag, why pass it in as a parameter? Because the caller may not want to respect the flag's value (maybe for tests or something). 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 You simply can't call asynchronous functions with Execute() - there's no callback provided to the method invocation on line 108. ExecuteAsync() is in the next patch. There's no use cases for it in this one. -- 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
