You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Fengling Wang (Code Review)" <ge...@cloudera.org> on 2018/05/07 20:39:38 UTC
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Fengling Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10332
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the browser supports the
gzip compression and when the user is not using curl.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/server/webserver.cc
1 file changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/1
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 1
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 9:
(1 comment)
Almost there: just one more nit.
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/server/webserver.cc@490
PS9, Line 490: if (zlib::Compress(Slice(full_content), &oss).ok()) {
: full_content = oss.str();
: is_compressed = true;
: } else {
: LOG(WARNING) << "Could not compress output";
: }
nit: it might be useful to output information from the error, e.g.
Status s = zlib::Compress(...);
if (s.ok()) {
...
} else {
LOG(WARNING) << "could not compress output: " << s.ToString();
}
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 9
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 22:00:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#11).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 116 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/11
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 11
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/util/curl_util.h@44
PS9, Line 44: Status FetchURL(const std::string& url,
No quite sure whether it was worth moving the comment from this place to the DoRequest() doc?
Any rationale behind?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 12
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 18 May 2018 19:21:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#4).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the gzip compression is
accepted by the caller.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 90 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/4
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 4
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#5).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the gzip compression is
accepted by the caller.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 87 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/5
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 5
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#9).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 110 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/9
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 9
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 10
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 23:46:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 3:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h@380
PS3, Line 380: enable and unenable
drop
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h@381
PS3, Line 381: if (compression_enabled) {
: s = curl.FetchURL(url, &dst, {"Accept-Encoding: gzip"});
: } else {
: s = curl.FetchURL(url, &dst);
: }
you could use the ternary '? :' operator for brevity here.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@22
PS3, Line 22: #include <gflags/gflags.h>
: #include <gflags/gflags_declare.h>
: #include <glog/logging.h>
: #include <gtest/gtest.h>
: #include <curl/curl.h>
re-order to be in alphabetic order
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@154
PS3, Line 154: size_t WriteCallback(void* buffer, size_t size, size_t nmemb, void* user_ptr) {
: size_t real_size = size * nmemb;
: faststring* buf = reinterpret_cast<faststring*>(user_ptr);
: CHECK_NOTNULL(buf)->append(reinterpret_cast<const uint8_t*>(buffer), real_size);
: return real_size;
: }
Is it used anywhere at all?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@168
PS3, Line 168: zlib::Uncompress(Slice(buf_.ToString()), &oss);
Check for return status, i.e. wrap it into ASSERT_OK()
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@180
PS3, Line 180: gzip"
To test that the server's code extracts the supported encoding properly, also add other encodings into the list, e.g. 'deflate', 'bzip2', 'xz'.
In addition to that, have an additional case that specifies list of encodings that is not supported by Kudu's internal Web server. For example, make a request with 'Accept-Encoding: megaturbogzip' and make sure the server does not return the gzipped content, because the server does not support the 'megaturbogzip' encoding.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@181
PS3, Line 181: Content-Encoding:
As a general note, HTTP header tags (or header names) are case-insensitive and the number of spaces after the column is arbitrary.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@184
PS3, Line 184: unenabled
disabled?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@188
PS3, Line 188: Check
Check for the
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@189
PS3, Line 189: ASSERT_STR_CONTAINS(buf_.ToString(), "X-Frame-Options: DENY");
Not sure whether this needs to be covered here. Maybe, just check for one of the mandatory headers for HTTP/1.1, like 'Host' ?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@191
PS3, Line 191: Check
ditto
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc@484
PS3, Line 484: accept_encoding_str
What if one of the supported encodings in the request were 'mygzip'?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc@483
PS3, Line 483: bool is_compressed = false;
: if (accept_encoding_str
What about:
const bool is_compressed = accept_encoding_str && strstr(...);
if (is_compressed) {
...
}
?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.h@43
PS3, Line 43: // Any existing data in the buffer is replaced.
Add a note about additional headers in the optional parameter.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: curl_slist *
Style: surrent code conventions dictate that it should be
struct curl_slist* chunk = nullptr;
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: chunk
What happens if this function returns earlier than calling curl_slist_free_all(chunk)?
Consider using unique_ptr with custom deleter to wrap curl_slist. You can see examples at https://en.cppreference.com/w/cpp/memory/unique_ptr
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: chunk
Why that name for the variable? Why not something meaningful like 'curl_headers' or alike?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@89
PS3, Line 89: const std::string &header : headers
const auto& header : headers
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@90
PS3, Line 90: chunk
Check for nullptr in case of an error.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 3
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 23:51:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 8
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:40:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#12).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 113 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/12
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 12
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 13
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 18 May 2018 21:18:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#3).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the gzip compression is
enabled from the client request headers.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 90 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/3
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 3
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 4:
(4 comments)
There seems to be no double-compression? So I called "http://localhost:8051/tracing/json/end_recording_compressed", and then got a gz file, after I decompressed it, it showed a content that makes sense.
Thanks!
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG@9
PS2, Line 9: gzip co
> nit: client
Done
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG@10
PS2, Line 10: caller.
> What's wrong with curl in that perspective?
My fault. Cuz a plain curl usually doesn't specify Accept-Encoding. I've changed the commit msg.
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc@481
PS2, Line 481: accepte
> nit: accepted by the caller ?
Done
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc@495
PS2, Line 495: "Content-Encoding: gzip\r\n";
> why not just
Done
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 4
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 10 May 2018 22:52:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG@9
PS6, Line 9: gzi
> nit: Extra "the".
Done
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG@10
PS6, Line 10: client
> nit: I think the http terminology is "client" or "requestor".
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@188
PS3, Line 188: Should
> Nope, I think in that case it's better just to keep it consistent.
Done
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@160
PS6, Line 160: deflate, br, gzip
> Because I'm paranoid, could you put gzip second or later in the list, to te
Done
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@183
PS6, Line 183: "Content-Type:");
> This is the status line; not a header. Also, neither this nor headers will
Done
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@195
PS6, Line 195: compression
> nit: Extra 's'.
Done
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver.cc@485
PS6, Line 485: ctor<string> encodings = strings::Spl
> We have gutil functions to make this easier. Check out
Thought about this but didn't search thoroughly. Thanks for reminding.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/util/curl_util.h@45
PS6, Line 45: e.g.
> nit: I think 'e.g.' is more appropriate here because that's just an example
Oh yes. Sorry it was a typo.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/util/curl_util.h@46
PS6, Line 46: Status FetchURL(const std::string& url,
> warning: function 'kudu::EasyCurl::FetchURL' has a definition with differen
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@90
PS3, Line 90: to cl
> I meant curl_slist_append() returns nullptr if anything went wrong. I thin
I see.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 15 May 2018 17:56:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 8:
(2 comments)
A couple of nits
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/integration-tests/linked_list-test-util.h@379
PS8, Line 379: Status s;
: // Switch compression back and forth.
: s = compression_enabled
nit: why not to combine declaration with initialization here?
Status s = ...
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/server/webserver.cc@490
PS8, Line 490: zlib::Compress(Slice(full_content), &oss);
What if it returns non-OK status? Maybe, then do LOG(WARNING) and return at this point (like it's done at line 444)?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 8
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 18:48:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed a vote on this change.
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 13
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc@96
PS7, Line 96: CHECK_NOTNULL(curl_headers);
> Opps my bad. And that's not leftover debugging. Maybe
By choke I mean would curl_slist_append(ch, s) crash/SIGSEGV if ch is a null pointer. If it did, the CHECK's don't help because we'd crash before them.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:06:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#8).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 108 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/8
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 8
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 2:
(4 comments)
I think it's worth adding test for this new piece of functionality. Maybe, just add one into the set of tests in webserver-test.cc?
Also, how does it work the already existing compression for the end-point like /tracing/json/end_recording_compresse ? Does it do double-compression in that case?
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG@9
PS2, Line 9: browser
nit: client
It might be not a browser, but just some tool.
http://gerrit.cloudera.org:8080/#/c/10332/2//COMMIT_MSG@10
PS2, Line 10: and when the user is not using curl
What's wrong with curl in that perspective?
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc@481
PS2, Line 481: allowed
nit: accepted by the caller ?
http://gerrit.cloudera.org:8080/#/c/10332/2/src/kudu/server/webserver.cc@495
PS2, Line 495: Substitute("Content-Encoding: $0\r\n", "gzip");
why not just
"Content-Encoding: gzip\r\n"
?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 2
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 07 May 2018 22:55:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 6:
(19 comments)
Plz ignore patch set 5.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h@380
PS3, Line 380: compression back and
> drop
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/integration-tests/linked_list-test-util.h@381
PS3, Line 381: s = compression_enabled ? curl.FetchURL(url, &dst, {"Accept-Encoding: gzip"})
: : curl.FetchURL(url, &dst);
: compression_enabled = !compression_enabled;
: if (s.ok()) {
:
> you could use the ternary '? :' operator for brevity here.
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@22
PS3, Line 22: #include <curl/curl.h>
: #include <gflags/gflags.h>
: #include <gflags/gflags_declare.h>
: #include <glog/logging.h>
: #include <gtest/gtest.
> re-order to be in alphabetic order
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@154
PS3, Line 154: TEST_F(WebserverTest, TestHttpCompression) {
: string url = strings::Substitute("http://$0/", addr_.ToString());
: std::ostringstream oss;
: string decoded_str;
:
:
> Is it used anywhere at all?
Oops sorry, forgot to remove it.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@168
PS3, Line 168:
> Check for return status, i.e. wrap it into ASSERT_OK()
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@180
PS3, Line 180:
> To test that the server's code extracts the supported encoding properly, al
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@181
PS3, Line 181:
> As a general note, HTTP header tags (or header names) are case-insensitive
I see.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@184
PS3, Line 184:
> disabled?
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@188
PS3, Line 188: Should
> Check for the
I followed the comment style from the 'TestIndexPage' above. Do I need to change that one also?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@189
PS3, Line 189: ASSERT_STR_CONTAINS(buf_.ToString(), "Kudu");
> Not sure whether this needs to be covered here. Maybe, just check for one
Done. 'Host' is in request headers but not in response headers tho?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc@484
PS3, Line 484:
> What if one of the supported encodings in the request were 'mygzip'?
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver.cc@483
PS3, Line 483: bool is_compressed = false;
: if (accept_encoding_str
> What about:
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.h@43
PS3, Line 43: // Any existing data in the buffer is replaced.
> Add a note about additional headers in the optional parameter.
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: headers if s
> Style: surrent code conventions dictate that it should be
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: pecif
> Why that name for the variable? Why not something meaningful like 'curl_he
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: pecif
> Ah, it seems MakeScopedCleanup would fit better in here. Anyway, make sure
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: pecif
> What happens if this function returns earlier than calling curl_slist_free_
I see. Thanks for reminding.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@89
PS3, Line 89: t curl_slist* curl_headers = nullpt
> const auto& header : headers
Done
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@90
PS3, Line 90: to cl
> Check for nullptr in case of an error.
Which one? I think it's ok for the 'chunk' to be null?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 11 May 2018 21:30:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc@96
PS7, Line 96: CHECK_NOTNULL(curl_headers);
> nit: is this leftover debugging? Would curl_slist_append choke on a NULL cu
Opps my bad. And that's not leftover debugging. Maybe
curl_headers = CHECK_NOTNULL(curl_slist_append(curl_headers, header.c_str()));
is better?
And what do you mean the choking?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:04:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#6).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the gzip compression is
accepted by the caller.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 111 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/6
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 11:
Fixed conflicts and rebased.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 11
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 17 May 2018 20:35:49 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(1 comment)
LGTM. One nit / question.
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc@96
PS7, Line 96: CHECK_NOTNULL(curl_headers);
nit: is this leftover debugging? Would curl_slist_append choke on a NULL curl_headers on the previous line anyway?
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 15:52:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 10:
(1 comment)
Thanks!
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/server/webserver.cc@490
PS9, Line 490: Status s = zlib::Compress(Slice(full_content), &oss);
: if (s.ok()) {
: full_content = oss.str();
: is_compressed = true;
: } else {
:
> nit: it might be useful to output information from the error, e.g.
Done
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 10
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 22:20:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#10).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 113 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/10
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 10
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 8:
Just one-line change on curl_util.cc Line95.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 8
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:16:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc@96
PS7, Line 96: CHECK_NOTNULL(curl_headers);
> Oh I see. It seems fine? It's based on the example I followed:
(thumbsup)
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:10:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 6:
(4 comments)
A few nits, but overall it's good.
Also, please address the automatically generated feedback from the Tidy Bot.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@188
PS3, Line 188: Should
> I followed the comment style from the 'TestIndexPage' above. Do I need to c
Nope, I think in that case it's better just to keep it consistent.
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/server/webserver-test.cc@189
PS3, Line 189: ASSERT_STR_CONTAINS(buf_.ToString(), "Kudu");
> Done. 'Host' is in request headers but not in response headers tho?
Woops, indeed. Sorry for messing it up.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/util/curl_util.h@45
PS6, Line 45: i.e.
nit: I think 'e.g.' is more appropriate here because that's just an example, right?
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@90
PS3, Line 90: to cl
> Which one? I think it's ok for the 'chunk' to be null?
I meant curl_slist_append() returns nullptr if anything went wrong. I think it's worth handling that situation here as well.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 14 May 2018 18:01:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/3/src/kudu/util/curl_util.cc@88
PS3, Line 88: chunk
> What happens if this function returns earlier than calling curl_slist_free_
Ah, it seems MakeScopedCleanup would fit better in here. Anyway, make sure there is no memory leak even if this method returns early.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 3
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 11 May 2018 00:13:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#2).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when the browser supports the
gzip compression and when the user is not using curl.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/server/webserver.cc
1 file changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/2
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 2
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc
File src/kudu/util/curl_util.cc:
http://gerrit.cloudera.org:8080/#/c/10332/7/src/kudu/util/curl_util.cc@96
PS7, Line 96: CHECK_NOTNULL(curl_headers);
> By choke I mean would curl_slist_append(ch, s) crash/SIGSEGV if ch is a nul
Oh I see. It seems fine? It's based on the example I followed:
https://curl.haxx.se/libcurl/c/httpcustomheader.html
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 16:09:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 10
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 23:37:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG@9
PS6, Line 9: the
nit: Extra "the".
http://gerrit.cloudera.org:8080/#/c/10332/6//COMMIT_MSG@10
PS6, Line 10: caller
nit: I think the http terminology is "client" or "requestor".
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@160
PS6, Line 160: gzip, deflate, br
Because I'm paranoid, could you put gzip second or later in the list, to test the server skips encodings the server doesn't support.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@175
PS6, Line 175: megaturbogzip
:)
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@183
PS6, Line 183: "HTTP/1.1 200 OK"
This is the status line; not a header. Also, neither this nor headers will be compressed, so I expect this test would succeed regardless of compression.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver-test.cc@195
PS6, Line 195: compressions
nit: Extra 's'.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver.cc@61
PS6, Line 61: struct sockaddr_in;
> warning: invalid case style for class 'sockaddr_in' [readability-identifier
You can ignore this one.
http://gerrit.cloudera.org:8080/#/c/10332/6/src/kudu/server/webserver.cc@485
PS6, Line 485: stringstream ss(accept_encoding_str);
We have gutil functions to make this easier. Check out
- StripWhiteSpace in gutill/strings/strip.h
- Split in gutil/strings/split.h
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 14 May 2018 23:07:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#13).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 113 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/13
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 13
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 11:
Plz ignore patch 11
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 11
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 17 May 2018 20:42:32 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/10332/9/src/kudu/util/curl_util.h@44
PS9, Line 44: // The optional param 'headers' holds additional headers.
> No quite sure whether it was worth moving the comment from this place to th
Opps sorry. It was a typo when I was trying to rebase.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 9
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 18 May 2018 19:42:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 9:
> (1 comment)
>
> Almost there: just one more nit.
Also, it's necessary to fix IWYU warnings:
http://jenkins.kudu.apache.org/job/kudu-gerrit/13471/BUILD_TYPE=IWYU/artifact/build/latest/test-logs/iwyu.log
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 9
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 22:02:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 13: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 13
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 18 May 2018 21:23:07 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 10:
Looks like you'll have to rebase on master before submitting. I'll forward the +2.
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 10
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 17 May 2018 17:29:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/integration-tests/linked_list-test-util.h
File src/kudu/integration-tests/linked_list-test-util.h:
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/integration-tests/linked_list-test-util.h@379
PS8, Line 379: Status s;
: // Switch compression back and forth.
: s = compression_enabled
> nit: why not to combine declaration with initialization here?
Done
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
http://gerrit.cloudera.org:8080/#/c/10332/8/src/kudu/server/webserver.cc@490
PS8, Line 490: zlib::Compress(Slice(full_content), &oss);
> What if it returns non-OK status? Maybe, then do LOG(WARNING) and return a
Done
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 8
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 16 May 2018 21:08:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Fengling Wang (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10332
to look at the new patch set (#7).
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 109 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/10332/7
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 7
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2035 Enable HTTP compression for all webserver's paths
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10332 )
Change subject: KUDU-2035 Enable HTTP compression for all webserver's paths
......................................................................
KUDU-2035 Enable HTTP compression for all webserver's paths
This patch enables HTTP compression when gzip compression is
accepted by the client.
Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Reviewed-on: http://gerrit.cloudera.org:8080/10332
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
5 files changed, 113 insertions(+), 7 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Will Berkeley: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7c6db24b0fd2cbcca8a554460d310bd39ee5c071
Gerrit-Change-Number: 10332
Gerrit-PatchSet: 14
Gerrit-Owner: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>