Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14573 )
Change subject: webserver: add support for Knox URL rewriting ...................................................................... Patch Set 6: (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: // A web server is responsible for escaping illegal URL characters in all : // links embedded in a response. In Kudu's web UI, a prime example is > As with regex parsing, it'd be nice if you could comment what this "matches Done http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@119 PS5, Line 119: string encoded_query_param_str = JoinStrings(encoded_query_params, "&"); : > nit: for anyone who isn't particularly well-versed in Knox, it isn't super I'd rather sprinkle comments next to code instead of a big summary as I find that more readable. Hopefully the additions I've made help. http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@158 PS5, Line 158: ack(StrCat("--webserver_private_key_file=", pk_file)); > Why are the WALs important? If it's so there's a meaningful log anchoring p Yeah, it's so the anchors get populated. I'll update. http://gerrit.cloudera.org:8080/#/c/14573/5/src/kudu/integration-tests/webserver-crawl-itest.cc@219 PS5, Line 219: > Is the point of Knoxifying the link purely to match what knox does? Does th If we don't Knoxify the link, we can't properly visit /threadz for a group with a space in its name (e.g. "acceptor pool"), as nobody involved in the receipt of the link (the web UI or the test) has encoded the space in the query parameter values. This is the difference between "/threadz?group=acceptor pool" and "/threadz?group=acceptor%20pool". The former yields a "group not found" error while the latter works. I'll try to update the comments to reflect this. 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: ffect rendering > nit: doc how this is used? I don't want it to be stale so I doc'ed it in a somewhat generic way. 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: > nit: the "why" is less clear than the "what" here IMO. Could you comment wh It's a boring reason but sure I'll doc it. 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: > Why don't we need to check whether we're going through Knox here? Good question. The answer is that we don't actually need to encode at all because the tablet ID is already URL-compliant (i.e. comprises alphanumeric characters or one of '-', '.', '_', or '~'). The URL encoding dates back to 2013, and maybe back then we didn't know (or weren't sure) that tablet IDs would be hexadecimal UUIDs. Elsewhere (tablet.mustache) I see we recognize this fact and don't bother URL-encoding tablet IDs, so I'll drop it here too. Oh, and now that mustache is recursively looking for variables, we don't need to duplicate "id" in the "link" object when it's available in the "replica" object (the parent of "link"). -- 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: 6 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 05:12:30 +0000 Gerrit-HasComments: Yes
