Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4786: Clean up how ImpalaServers are created ......................................................................
Patch Set 2: (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: > what is the purpose of this? I think we should avoid shared_from_this() unl Since the Thrift classes expect a boost::shared_ptr, we need to pass in a boost::shared_ptr<ImpalaServer> to the interface of ImpalaHiveServer2ServiceProcessor, etc. shared_from_this() is basically a shared_ptr version of the 'this' pointer. So this looks like the cleanest way to do this. http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h File be/src/service/impala-server.h: PS1, Line 123: thrift_ > is that the thrift impala internal service port (as opposed to to krpc veri It's the thrift internal service port. I renamed it to thrift_be_port. Line 999: /// Init() were <= 0. > can you comment on who owns these? even better, if this class owns them, us Changed to scoped_ptr. PS1, Line 1002: ptr<Thrift > since krpc is on the way, how about thrift_be_server_? this goes away once Yes, that's right. I've renamed it. 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 al Yes, they are a consequence of the Thrift interface expecting a boost::shared_ptr. The Thrift classes are: ImpalaServiceProcessor, ImpalaHiveServer2ServiceProcessor, etc. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
