Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/7901 )
Change subject: IMPALA-4670: Introduces RpcMgr class ...................................................................... Patch Set 8: Code-Review+2 (7 comments) http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/rpc/rpc-mgr.h@90 PS8, Line 90: thrads threads http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/rpc/rpc-mgr.h@90 PS8, Line 90: some this is kind of a detail but "some" implies that requests can bypass the queue when there is an available service thread. is that actually the case? 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: /// terminated. RpcMgr::Init() and RpcMgr::Shutdown() are not thread safe. > There is none actually. The assumption is that Init() is called from the si Okay. The case I'm worried about with Shutdown() is if this happens while RPCs are in flight, what happens. It'd be nice to move Impala toward having clean exit behavior, rather than adding more things that could crash when e.g. the impalad process gets a term signal. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@133 PS6, Line 133: > Normally, we shouldn't shutdown as there is really no clean way to shut dow See my comment above about clean process shutdown. Not sure there's anything to tackle now but let's think it through. http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@156 PS6, Line 156: > This is similar to a shared_ptr in a way. KRPC messenger internally uses a Not suggesting we change it, just want to check that the scoped_refptr is dictated by the krpc layer API. http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/scheduling/scheduler.cc@72 PS8, Line 72: is delete http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/service/impala-server.cc@1662 PS8, Line 1662: } okay. or maybe the exec env should just have the resolved IP in it from ExecEnv::StartServices()? -- 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: 8 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: Mon, 02 Oct 2017 21:42:03 +0000 Gerrit-HasComments: Yes
