Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14573 )
Change subject: webserver: add support for Knox URL rewriting ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc File src/kudu/integration-tests/webserver-crawl-itest.cc: http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@93 PS5, Line 93: // When Knox rewrites an HTTP response, it encodes query parameter values in : // any URLs in the response. To impersonate Knox we must do the same. As with regex parsing, it'd be nice if you could comment what this "matches" (and replaces) http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@119 PS5, Line 119: : TEST_P(WebserverCrawlITest, TestAllWebPages) { nit: for anyone who isn't particularly well-versed in Knox, it isn't super obvious what verifications and rewritings we're doing in this crawling. Could you summarize what we're looking for in each link, what "Knoxifying" the link accomplishes, and why that's important? http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@158 PS5, Line 158: ' WALs. Why are the WALs important? If it's so there's a meaningful log anchoring page, maybe extend this comment to clarify? http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@219 PS5, Line 219: NO_FATALS(KnoxifyURL(&link)); Is the point of Knoxifying the link purely to match what knox does? Does this test any Kudu codepaths? http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.h@133 PS5, Line 133: WebRequest& req, nit: doc how this is used? http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/server/webserver.cc@543 PS5, Line 543: // Canonicalize header names to lower case. nit: the "why" is less clear than the "what" here IMO. Could you comment why we need to do this? http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/tserver/tserver_path_handlers.cc@347 PS5, Line 347: UrlEncodeToString(status.tablet_id())); Why don't we need to check whether we're going through Knox here? -- To view, visit http://gerrit.cloudera.org:8080/14573 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee92cb094b81609356acf858b7c549b6c281a7e5 Gerrit-Change-Number: 14573 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 01 Nov 2019 00:12:40 +0000 Gerrit-HasComments: Yes
