Matthew Jacobs has posted comments on this change. Change subject: Move Impala HTTP handlers to a separate class ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/3370/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS1, Line 316: http_handler_.reset(new ImpalaHttpHandler(this)); > It holds on to the pointer to 'this', i.e. the ImpalaServer that the handle But the lifetime of 'this' is already the lifetime of this scoped object (in fact just a tad longer, since http_hander_ gets destructed before 'this' becomes invalid). Couldn't RegisterHandlers even be a static method which takes both 'this' and the webserver and then needs no state? (I'm not saying we should do that since I see that would require more changes because ImpalaServer* has to be passed around more.) While this is a small thing, it's nice to remove any unnecessary state from ImpalaServer. -- To view, visit http://gerrit.cloudera.org:8080/3370 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8453b3367653914163ca6acae48e7605f73cc675 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
