Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8141 )
Change subject: [webui] Allow custom response codes and headers ...................................................................... Patch Set 6: (9 comments) I'm not an HTTP expert so it'd be good to find a reviewer who is (maybe Todd?). I know Impala also uses Squeasel; perhaps they even use this Webserver code. Could you check in with an Impala developer and see whether their Webserver already does this, or whether they'd be interested in this functionality? http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8141/6//COMMIT_MSG@14 PS6, Line 14: has have ("WebResponse and PrerenderedWebResponse" are plural) http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/master/master-path-handlers.cc File src/kudu/master/master-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/master/master-path-handlers.cc@242 PS6, Line 242: response respond http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.h@66 PS6, Line 66: // Register a route 'path' to be rendered via template. Since you're already updating these docs, could you explain what 'alias', 'is_styled' and 'is_on_nav_bar' do? http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@88 PS6, Line 88: switch (code) { Add a default case that crashes or something? Would be good to know if we've forgotten to update this switch when adding a new code. http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@452 PS6, Line 452: HttpResponseHeaders{} Just {} doesn't work? http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@469 PS6, Line 469: for (const auto& entry : resp.response_headers) { Should we enforce that response_headers doesn't include Content-Type, Content-Length, or X-Frame-Options? I understand it's sort of moot since no callback responds with headers right now, but what happens if a duplicate header is in the resposne? http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@486 PS6, Line 486: HttpResponseHeaders{} Just {} doesn't work? http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h File src/kudu/util/web_callback_registry.h: http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@66 PS6, Line 66: typedef std::map<std::string, std::string> HttpResponseHeaders; Is it important that the headers be sorted by key? Or can this be an unordered_map instead? http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/util/web_callback_registry.h@69 PS6, Line 69: // - 'status_code' determines the status code of the HTTP response. : // - 'response_headers' are additional headers added to the HTTP response. : // - 'output' is a JSON object to be rendered in a mustache template. Nit: move each of these so that it's just above the declaration of its field. Same for 'output' in PrerenderedWebResponse. -- To view, visit http://gerrit.cloudera.org:8080/8141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ff890785eeb2df3eed9e7c54d0daf760c8b3924 Gerrit-Change-Number: 8141 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 02 Oct 2017 21:33:14 +0000 Gerrit-HasComments: Yes
