Will Berkeley 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 let Impala people know about this patch. 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) Done 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 Done 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', ' Done 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'v Done, but I added outside the switch so the compiler will generate warnings for unhandled enum class members. http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@452 PS6, Line 452: HttpResponseHeaders{} > Just {} doesn't work? Done 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, Conte Yeah, I think we should check for it. A repeated header is only allowed if the header value is CSV type, and none of those are. http://gerrit.cloudera.org:8080/#/c/8141/6/src/kudu/server/webserver.cc@486 PS6, Line 486: HttpResponseHeaders{} > Just {} doesn't work? Done 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 unorde Done 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 fiel Done -- 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: Tue, 03 Oct 2017 22:38:23 +0000 Gerrit-HasComments: Yes
