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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 11 May 2018 21:30:59 +0000 Gerrit-HasComments: Yes
