Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/21920 )
Change subject: Implement path parameter handling in webserver ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/default_path_handlers.cc@443 PS7, Line 443: nit: remove extra newline http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver-test.cc@133 PS7, Line 133: nit: remove extra newline http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@596 PS7, Line 596: params nit: you can call it path_params to make it easier to understand. http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@615 PS7, Line 615: nit: start with match = false, and change the later assignments accordingly (in general to avoid false positives while searching) http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@617 PS7, Line 617: std::vector<std::string> handler_segments = SplitPath(path_handler.first); : double check: maybe add a third condition that handler_segments[i].size() >= 3. < and > are already 2 chars, for it to be a vaild segment identifier we need at least one more char. or you can just add a DCHEC() for this size checking before the if . http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@619 PS7, Line 619: if (handler_segments.size() != uri_segments.size()) { : continue; nit: std:string param_name = handler_segments[i].substr(1, str.size() - 2) http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@728 PS7, Line 728: // Should we render with css styles? since we are using c++17 if you fancy you can refactor this using string_views to make it more performant. http://gerrit.cloudera.org:8080/#/c/21920/7/src/kudu/server/webserver.cc@732 PS7, Line 732: return SQ_HANDLED_OK; : } : : std:: maybe it is better if we check this outside of SplitPath as its a different concern. maybe before this line in the above section: PathHandlerMap::const_iterator it = path_handlers_.find(request_info->uri); -- To view, visit http://gerrit.cloudera.org:8080/21920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id2085c55d3b86985c87c5fd5a8b48568f8e6fa63 Gerrit-Change-Number: 21920 Gerrit-PatchSet: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 31 Oct 2024 13:54:01 +0000 Gerrit-HasComments: Yes
