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-4786: Clean up how ImpalaServers are created
Date Mon, 18 Sep 2017 22:31:17 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: handler = shared_from_this();
what is the purpose of this? I think we should avoid shared_from_this() unless it's really
the clearest way to do something. could you elaborate on what this is all about?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: be_port
is that the thrift impala internal service port (as opposed to to krpc verison of that service),
or something else?


Line 999:   /// Init() were <= 0.
can you comment on who owns these? even better, if this class owns them, use scoped_ptr to
make that explicit (we just don't want to use shared_ptr, and shared ownership, generally
unless it really makes sense. but scoped_ptr, which implies single ownership, is fine.


PS1, Line 1002: be_server_
since krpc is on the way, how about thrift_be_server_? this goes away once we fully switch
ImpalaInternalService to krpc, is that right?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr<ImpalaServer> impala_server
why do we need shared ownership of this thing? who else needs to keep it alive beyond this
function scope? (maybe it is needed, but let's reason through that).

Oh, maybe this (and the one in ImpalaServer shared_from_this) is a consequence of ImpalaServer
being the thing that implements the thrift interface and thrift requires the shared_ptr?


-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message