impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class
Date Mon, 02 Oct 2017 21:42:03 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 21:42:03 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message