You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/06/11 00:15:34 UTC

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class 

Henry Robinson has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/3370

Change subject: Move Impala HTTP handlers to a separate class
......................................................................

Move Impala HTTP handlers to a separate class

HTTP handler callbacks take a lot of header room, partly because they
usually have their JSON output documented literally. This patch moves
ImpalaServer's HTTP handlers out of ImpalaServer itself, into a friend
class.

Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
---
M be/src/service/CMakeLists.txt
R be/src/service/impala-http-handler.cc
A be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 266 insertions(+), 222 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/70/3370/1
-- 
To view, visit http://gerrit.cloudera.org:8080/3370
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson 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));
> But the lifetime of 'this' is already the lifetime of this scoped object (i
I see what you mean. The other bit of state that the callbacks have, implicitly, is a pointer to their containing object (i.e. the ImpalaHttpHandler). They don't dereference that right now, but there's nothing to stop one of them doing so in the future, and it would be a bit surprising to find a crash because there are methods being called on an object which has since been destroyed. I thought about doing this, but I think it's better not to surprise people with unexpected lifetimes, even if it would be safe here.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson 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 doesn't look like ImpalaHttpHandler holds onto any state that needs to b
It holds on to the pointer to 'this', i.e. the ImpalaServer that the handlers use to respond to requests.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3736: Move Impala HTTP handlers to a separate class

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3736: Move Impala HTTP handlers to a separate class
......................................................................


Patch Set 2: Code-Review+2 Verified+1

This passed a full build:

http://sandbox.jenkins.cloudera.com/job/impala-umbrella-build-and-test/1726/consoleFull

Since this is only really moving code around, I'll give it a +2.

-- 
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: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: Move Impala HTTP handlers to a separate class
......................................................................


Patch Set 1: Code-Review+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));
> I see what you mean. The other bit of state that the callbacks have, implic
Ah, ok, makes sense. Thanks.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-CR](cdh5-trunk) IMPALA-3736: Move Impala HTTP handlers to a separate class

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-3736: Move Impala HTTP handlers to a separate class
......................................................................


IMPALA-3736: Move Impala HTTP handlers to a separate class

HTTP handler callbacks take a lot of header room, partly because they
usually have their JSON output documented literally. This patch moves
ImpalaServer's HTTP handlers out of ImpalaServer itself, into a friend
class.

Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
Reviewed-on: http://gerrit.cloudera.org:8080/3370
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Henry Robinson <he...@cloudera.com>
---
M be/src/service/CMakeLists.txt
R be/src/service/impala-http-handler.cc
A be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 269 insertions(+), 222 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-CR](cdh5-trunk) IMPALA-3736: Move Impala HTTP handlers to a separate class

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/3370

to look at the new patch set (#2).

Change subject: IMPALA-3736: Move Impala HTTP handlers to a separate class
......................................................................

IMPALA-3736: Move Impala HTTP handlers to a separate class

HTTP handler callbacks take a lot of header room, partly because they
usually have their JSON output documented literally. This patch moves
ImpalaServer's HTTP handlers out of ImpalaServer itself, into a friend
class.

Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
---
M be/src/service/CMakeLists.txt
R be/src/service/impala-http-handler.cc
A be/src/service/impala-http-handler.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 269 insertions(+), 222 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/70/3370/2
-- 
To view, visit http://gerrit.cloudera.org:8080/3370
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8453b3367653914163ca6acae48e7605f73cc675
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-CR](cdh5-trunk) Move Impala HTTP handlers to a separate class

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
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 doesn't look like ImpalaHttpHandler holds onto any state that needs to be alive after RegisterHandlers() returns. Can we avoid making this a member and just create it on the stack here?


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes