Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14339 )
Change subject: IMPALA-8899: Add cookie support to the webui ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/14339/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14339/4//COMMIT_MSG@13 PS4, Line 13: It also fixes an issue where some clients may return the cookie value Do you have an example of a client that does this? Might be useful for future reference. http://gerrit.cloudera.org:8080/#/c/14339/4//COMMIT_MSG@19 PS4, Line 19: 8898 Also include in the first line since it fixes it too? http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver-test.cc File be/src/util/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver-test.cc@336 PS4, Line 336: checkAuthMetrics nit: CheckAuthMetrics? http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver-test.cc@379 PS4, Line 379: if (RunShellProcess("curl --version", &curl_output) I guess this is better than just having it commented out, so it is a step in the right direction, but the potential silent loss of test coverage is not idea. I guess you're thinking that it's not worth blocking this change to deal with the toolchain stuff? I don't think it's unreasonable to require that test environments have curl available, FWIW, even if it's not added to the toolchain. http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h File be/src/util/webserver.h: http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h@190 PS4, Line 190: struct sq_request_info* request_info, std::vector<std::string>& response_headers); nit: Pass mutable param by pointer http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h@241 PS4, Line 241: bool use_cookies_; Maybe worth having a brief comment? http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h@243 PS4, Line 243: bool metrics_enabled_; If it's not too much work, it would be nice to enforce that metrics are always enabled, just so that the backend tests match the real tests as much as possible (and generally reduce the number of code paths). I took a quick look and it seemed like it would be possible to create a new metrics object for each test (it's been messier in some other backend tests where there are static functions and singleton objects involved, but it seems like it would be clean here). http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.cc@523 PS4, Line 523: vookie typo http://gerrit.cloudera.org:8080/#/c/14339/4/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/14339/4/common/thrift/metrics.json@2535 PS4, Line 2535: "description": "The number of HTTP connection requests to this daemon's webserver that were successfully authenticated with Kerberos", Nice... good to see some actual metrics for the webserver :) -- To view, visit http://gerrit.cloudera.org:8080/14339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30788e0539627ee6154ad8183b124947c5da8ef4 Gerrit-Change-Number: 14339 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 07 Oct 2019 22:44:05 +0000 Gerrit-HasComments: Yes
