Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14339 )
Change subject: IMPALA-8899, IMPALA-8898: Add cookie support to the webui ...................................................................... Patch Set 5: (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, for example Knox, may > Do you have an example of a client that does this? Might be useful for futu Done 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? Done 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: > nit: CheckAuthMetrics? Done http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver-test.cc@379 PS4, Line 379: > I guess this is better than just having it commented out, so it is a step i Right, I would prefer to get this patch in and worry about the toolchain stuff later. Of course, it would be pretty easy to add curl to bootstrap_system, and experimentally I believe that its the case that curl is already available in most of our usual automated testing environments. The issue is that you're not necessarily getting the right version of curl with the necessary options. In particular, experimentally I think its the case that the default curl that you'll get on centos7 doesn't have SPNEGO support, which the default curl on ubuntu16 does. I'll double check if that's the case. That of course wouldn't be an issue if we go the toolchain route. Maybe you think its worth just going ahead and adding curl to bootstrap_system, though? 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 Done http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h@241 PS4, Line 241: /// If true and SPNEGO is in use, cookies will be used for authentication. > Maybe worth having a brief comment? Done http://gerrit.cloudera.org:8080/#/c/14339/4/be/src/util/webserver.h@243 PS4, Line 243: > If it's not too much work, it would be nice to enforce that metrics are alw Done 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: atus.G > typo Done 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 :) Done -- 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: 5 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: Tue, 08 Oct 2019 18:20:27 +0000 Gerrit-HasComments: Yes
