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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
