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 <aser...@cloudera.com> Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 10 May 2018 23:51:23 +0000 Gerrit-HasComments: Yes
