impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class
Date Thu, 05 Oct 2017 17:43:25 GMT
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/7901 )

Change subject: IMPALA-4670: Introduces RpcMgr class
......................................................................


Patch Set 6:

(5 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: t th
> this is kind of a detail but "some" implies that requests can bypass the qu
changed to "new requests"


http://gerrit.cloudera.org:8080/#/c/7901/8/be/src/rpc/rpc-mgr.h@90
PS8, Line 90: l fail
> threads
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: ///
> Okay. The case I'm worried about with Shutdown() is if this happens while R
The shutdown of the ServicePool will wait for the exit of all service threads so all incoming
RPCs being processed would have been replied to before shutdown. All pending incoming RPCs
in the service queue will also be replied to too.

When shutting down the messenger, all thread pools (acceptor threads, negotiation threads
and reactor threads) encapsulated by it will be shut down too.


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: r_.
> delete
Done


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:   item.key = local_backend_id;
> okay. or maybe the exec env should just have the resolved IP in it from Exe
I was wondering if the old code intentionally did an IP address resolution above to capture
any change from hostname to IP address ? I suppose it's a fair assumption that any IP address
change will involve restarting the cluster so it should be fine to cache the IP.



-- 
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 <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: Thu, 05 Oct 2017 17:43:25 +0000
Gerrit-HasComments: Yes

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