You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/10/15 05:20:41 UTC

[kudu-CR] webserver: enable HTTP keep-alive 

Hello Alexey Serbin, Andrew Wong,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: webserver: enable HTTP keep-alive
......................................................................

webserver: enable HTTP keep-alive

This is a port of IMPALA-8869. The motivation is, to quote from IMPALA-8869:
"...we mishandle HTTP keep-alive semantics when returning a 401 because we
close the connection but don't return a 'Connection: close' header, even
though we're using HTTP/1.1 where keep-alive is assumed, which can cause
clients to incorrectly believe that the connection has remained open."

More tangibly, this leads to issues when proxying via Apache Knox.
Specifically, every other request fails because the browser expected its
connection to remain open, but Kudu closed it.

The main challenge is that now we need to buffer up all parts of the
response to avoid triggering a combination of Nagle's algorithm and TCP
delayed acks, which add ~40ms to the RTT. To be safe, I refactored all
response handling to go through a single function.

Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/web_callback_registry.h
6 files changed, 123 insertions(+), 58 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/14440/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 5: Verified+1

Overriding Jenkins, unrelated test failures.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 09:02:18 +0000
Gerrit-HasComments: No

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: webserver: enable HTTP keep-alive
......................................................................

webserver: enable HTTP keep-alive

This is a port of IMPALA-8869. The motivation is, to quote from IMPALA-8869:
"...we mishandle HTTP keep-alive semantics when returning a 401 because we
close the connection but don't return a 'Connection: close' header, even
though we're using HTTP/1.1 where keep-alive is assumed, which can cause
clients to incorrectly believe that the connection has remained open."

More tangibly, this leads to issues when proxying via Apache Knox.
Specifically, every other request fails because the browser expected its
connection to remain open, but Kudu closed it.

The main challenge is that now we need to buffer up all parts of the
response to avoid triggering a combination of Nagle's algorithm and TCP
delayed acks, which add ~40ms to the RTT. To be safe, I refactored all
response handling to go through a single function. Since I was knee deep in
refactoring, I took the opportunity to fix the "SPNEGO header not returned
with 200 response" issue.

Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
---
M src/kudu/master/master_path_handlers.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/pprof_path_handlers.cc
M src/kudu/server/rpcz-path-handler.cc
M src/kudu/server/tracing_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/thread.cc
M src/kudu/util/web_callback_registry.h
13 files changed, 203 insertions(+), 177 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/14440/5
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................

webserver: enable HTTP keep-alive

This is a port of IMPALA-8869. The motivation is, to quote from IMPALA-8869:
"...we mishandle HTTP keep-alive semantics when returning a 401 because we
close the connection but don't return a 'Connection: close' header, even
though we're using HTTP/1.1 where keep-alive is assumed, which can cause
clients to incorrectly believe that the connection has remained open."

More tangibly, this leads to issues when proxying via Apache Knox.
Specifically, every other request fails because the browser expected its
connection to remain open, but Kudu closed it.

The main challenge is that now we need to buffer up all parts of the
response to avoid triggering a combination of Nagle's algorithm and TCP
delayed acks, which add ~40ms to the RTT. To be safe, I refactored all
response handling to go through a single function. Since I was knee deep in
refactoring, I took the opportunity to fix the "SPNEGO header not returned
with 200 response" issue.

Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Reviewed-on: http://gerrit.cloudera.org:8080/14440
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/master/master_path_handlers.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/pprof_path_handlers.cc
M src/kudu/server/rpcz-path-handler.cc
M src/kudu/server/tracing_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/thread.cc
M src/kudu/util/web_callback_registry.h
13 files changed, 203 insertions(+), 177 deletions(-)

Approvals:
  Adar Dembo: Verified
  Thomas Tauber-Marshall: Looks good to me, but someone else must approve
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Alexey Serbin, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: webserver: enable HTTP keep-alive
......................................................................

webserver: enable HTTP keep-alive

This is a port of IMPALA-8869. The motivation is, to quote from IMPALA-8869:
"...we mishandle HTTP keep-alive semantics when returning a 401 because we
close the connection but don't return a 'Connection: close' header, even
though we're using HTTP/1.1 where keep-alive is assumed, which can cause
clients to incorrectly believe that the connection has remained open."

More tangibly, this leads to issues when proxying via Apache Knox.
Specifically, every other request fails because the browser expected its
connection to remain open, but Kudu closed it.

The main challenge is that now we need to buffer up all parts of the
response to avoid triggering a combination of Nagle's algorithm and TCP
delayed acks, which add ~40ms to the RTT. To be safe, I refactored all
response handling to go through a single function. Since I was knee deep in
refactoring, I took the opportunity to fix the "SPNEGO header not returned
with 200 response" issue.

Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
---
M src/kudu/master/master_path_handlers.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/pprof_path_handlers.cc
M src/kudu/server/rpcz-path-handler.cc
M src/kudu/server/tracing_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/thread.cc
M src/kudu/util/web_callback_registry.h
13 files changed, 198 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/14440/4
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc@140
PS2, Line 140: pair<string, string>
> Maybe, create a typedef for HTTP header and use it here and in other places
Eh, it's only used in two places in webserver.cc (this is a static function). I think I'll pass.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 03:49:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/14440/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14440/6//COMMIT_MSG@12
PS6, Line 12: keep-alive is assumed
> nit: maybe, add a reference to that, like https://tools.ietf.org/html/rfc20
That's not quite the right section of the reference though. Plus I'm quoting from IMPALA-8869 so I'd like to keep that quote pristine.


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@601
PS6, Line 601: stringstream
> nit: ostringstream ?
All the mustache code paths using stringstream because that's what mustache's RenderTemplate function expects.

I even teed up a patch in mustache to replace stringstream with ostringstream before realizing that Impala uses stringstreams almost exclusively, so such a patch, while nice for Kudu, would be frustrating for Impala.


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@682
PS6, Line 682: stringstream
> nit: ostringstream ?
Ack


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@757
PS6, Line 757: stringstream
> nit: ostringstream ?
Ack


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@777
PS6, Line 777: stringstream
> nit: ostringstream ?
Ack



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 18 Oct 2019 03:29:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 6: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 20:07:54 +0000
Gerrit-HasComments: No

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc@140
PS2, Line 140: pair<string, string>
> Eh, it's only used in two places in webserver.cc (this is a static function
SGTM



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 17:59:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Alexey Serbin, Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: webserver: enable HTTP keep-alive
......................................................................

webserver: enable HTTP keep-alive

This is a port of IMPALA-8869. The motivation is, to quote from IMPALA-8869:
"...we mishandle HTTP keep-alive semantics when returning a 401 because we
close the connection but don't return a 'Connection: close' header, even
though we're using HTTP/1.1 where keep-alive is assumed, which can cause
clients to incorrectly believe that the connection has remained open."

More tangibly, this leads to issues when proxying via Apache Knox.
Specifically, every other request fails because the browser expected its
connection to remain open, but Kudu closed it.

The main challenge is that now we need to buffer up all parts of the
response to avoid triggering a combination of Nagle's algorithm and TCP
delayed acks, which add ~40ms to the RTT. To be safe, I refactored all
response handling to go through a single function.

Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/web_callback_registry.h
6 files changed, 124 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/14440/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 03:49:12 +0000
Gerrit-HasComments: No

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/util/curl_util.cc@165
PS2, Line 165:   RETURN_NOT_OK(TranslateError(curl_easy_getinfo(curl_, CURLINFO_NUM_CONNECTS, &val)));
             :   num_connects_ = val;
Should this come after line 158 but before line 161?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 01:55:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/3/src/kudu/server/webserver.cc@509
PS3, Line 509:     if (it == path_handlers_.end()) {
> Btw, you may find that you need to fix this eventually - it was fixed in Im
We talked about this offline. Sounds like you found that Python's urllib ends up emitting this, and we have some important clients that use urllib.

I'll fix it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 23:18:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/util/curl_util.cc@165
PS2, Line 165:     return Status::RemoteError(strings::Substitute("HTTP $0", val));
             :   }
> Should this come after line 158 but before line 161?
I don't think it matters a whole lot either way, but I'll make the change.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 03:14:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 6: Verified+1

Overriding Jenkins, unrelated test failure.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 17 Oct 2019 19:55:47 +0000
Gerrit-HasComments: No

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/3/src/kudu/server/webserver.cc@509
PS3, Line 509:     // NOTE: According to the SPNEGO RFC (https://tools.ietf.org/html/rfc4559) it
Btw, you may find that you need to fix this eventually - it was fixed in Impala as a side effect of: https://gerrit.cloudera.org/#/c/14339/, which I'm not sure if you're planning on porting over



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 18:09:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/2/src/kudu/server/webserver.cc@140
PS2, Line 140: pair<string, string>
Maybe, create a typedef for HTTP header and use it here and in other places in webserver.{h,cc}?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 03:47:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Patch Set 6: Code-Review+2

(5 comments)

http://gerrit.cloudera.org:8080/#/c/14440/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14440/6//COMMIT_MSG@12
PS6, Line 12: keep-alive is assumed
nit: maybe, add a reference to that, like https://tools.ietf.org/html/rfc2068#section-19.7.1


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@601
PS6, Line 601: stringstream
nit: ostringstream ?


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@682
PS6, Line 682: stringstream
nit: ostringstream ?


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@757
PS6, Line 757: stringstream
nit: ostringstream ?


http://gerrit.cloudera.org:8080/#/c/14440/6/src/kudu/server/webserver.cc@777
PS6, Line 777: stringstream
nit: ostringstream ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 18 Oct 2019 01:14:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] webserver: enable HTTP keep-alive

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/14440 )

Change subject: webserver: enable HTTP keep-alive
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/14440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ic08ef5a268fdf6dea6a8c428b4ab8dac27418dd6
Gerrit-Change-Number: 14440
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)