Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7901 )
Change subject: IMPALA-4670: Introduces RpcMgr class ...................................................................... Patch Set 6: (16 comments) http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h File be/src/exec/kudu-util.h: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h@87 PS6, Line 87: /// Impala Status object. > maybe note that the returned error is always of type GENERAL, i.e. we don't Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc@65 PS6, Line 65: FLAGS_hostname > Do we want this to be the loopback address since this is only a test? It sees fine as this is closer to what we actually do in ExecEnv::StartServices(). http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc@184 PS6, Line 184: rpc_mgr_.Shutdown(); > We don't need to call Shutdown here and in the other tests right? Since it Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@76 PS6, Line 76: /// > what is the thread safety story of this class? it looks like initializing i There is none actually. The assumption is that Init() is called from the singleton ExecEnv when it's initialized and Shutdown() is called when the singleton ExecEnv is destroyed. Comments updated. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@89 PS6, Line 89: FIFO fixed-size queue > is that queue per service? and is there a queue for connection requests as There is one queue per service. Comment updated. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@89 PS6, Line 89: processed > that's talking about being processed by either acceptor or service, right? Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@104 PS6, Line 104: address > does that one have to be an IP addr or is hostname okay? Comments clarified. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@115 PS6, Line 115: being > begin Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@133 PS6, Line 133: /// All acceptor and reactor threads within the messenger are also shut down. > what happens to stuff in flight, or should we not care for some reason? Normally, we shouldn't shutdown as there is really no clean way to shut down Impala now. ServicePool internally will return errors to clients of all unprocessed requests. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@156 PS6, Line 156: scoped_refptr > I guess using that is kinda required by krpc? What does it do? ServicePool This is similar to a shared_ptr in a way. KRPC messenger internally uses a map of service-name to scoped_refptr<ServicePool>> to track all service pools. It seems safer and more consistent to use a scoped_refptr here to avoid accidentally accessing freed memory. In theory, we can make sure the service_pools_ is cleared before we call messenger_.UnregisterAllServices() but it still seems a bit dicey. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@169 PS6, Line 169: shared_ptr > I guess using shared_ptr here is required by the MessangerBuilder::Build(). Yes, required by MessengerBuilder::Build(). http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc@30 PS6, Line 30: using std::move; > why is that needed? Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h@135 PS6, Line 135: krpc_port > does that still need to be exposed if we have krpc_address()? And looking a Removed. Please take a look at the changed logic in impala-server.cc http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/scheduling/scheduler.cc@a73 PS6, Line 73: > maybe this comment is still useful in the new code (minus the KRPC bits) in Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc File be/src/util/network-util.cc: http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc@120 PS6, Line 120: Sockaddr sock; > okay to ignore, but i think keeping the kudu:: namespace here makes it easi Done http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc@218 PS6, Line 218: Sockaddr > same Done -- To view, visit http://gerrit.cloudera.org:8080/7901 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46 Gerrit-Change-Number: 7901 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Fri, 29 Sep 2017 07:02:01 +0000 Gerrit-HasComments: Yes
