Dan Hecht has posted comments on this change. Change subject: IMPALA-4786: Clean up how ImpalaServers are created ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 2078: } i think we still need to reset these pointers since they hold a shared_ptr to 'this'. Otherwise, we have a cycle and 'this' will never be destructed. For that reason, I'm not really sold that moving the ownership of the ThriftServer's into this class is an improvement (due to this ownership cycle), but as long as you add the comment in the header file about that, we can go forward. http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 999: /// Init() were <= 0. add to comment: Note that these hold a shared pointer to 'this', and so need to be reset() explicitly. -- 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
