Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: [WIP] KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 23: (8 comments) http://gerrit.cloudera.org:8080/#/c/21823/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21823/23//COMMIT_MSG@26 PS23, Line 26: No Security Measures Yet. This implementation does not : currently include authentication or authorization. You can remove this line, once the kdc tests are included. http://gerrit.cloudera.org:8080/#/c/21823/23//COMMIT_MSG@38 PS23, Line 38: You can mention at the end that there will be follow-up patches for: - writing adding ranger itest - swagger/docs http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/master.cc@135 PS23, Line 135: Enables REST API endpoints in the master server. "Enables REST API endpoints for metadata management in the master server." Maybe it is also worthwhile to mention that for this to work, the flag webserver_enabled must also be true. (just stating the obvious for users). http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/master.cc@136 PS23, Line 136: TAG_FLAG(enable_rest_api, unsafe); Since SPNEGO works now with the api, i think we can remove this line, right? http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/master.cc@137 PS23, Line 137: Looking at webserver_enabled flag, if that uses TAG_FLAG(webserver_enabled, advanced); its probably good to have it here as well. http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/rest_catalog-test.cc@1 PS23, Line 1: #include <regex.h> Please add the Apache license in the beginning of this files as well. http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/rest_catalog-test.cc@106 PS23, Line 106: static int FindColumnId(string const &schema_json) { : regex_t column_id_regex; : regmatch_t match[2]; : const char* pattern = "\\{\"id\":([0-9]+),\"name\":\"key\""; : if (regcomp(&column_id_regex, pattern, REG_EXTENDED) != 0) { : return -1; : } : if (regexec(&column_id_regex, schema_json.c_str(), 2, match, 0) == 0) { : std::string column_id = schema_json.substr(match[1].rm_so, match[1].rm_eo - match[1].rm_so); : regfree(&column_id_regex); : return std::stoi(column_id); : } : regfree(&column_id_regex); : return -1; : } nit: I just saw that we have "class KuduRegex " in src/kudu/util/regex.h. Maybe you can simplify this function by using that class. (its just a wrapper around regex.h) If this turns out to be too much work, just ignore this. http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/23/src/kudu/master/rest_catalog_path_handlers.cc@58 PS23, Line 58: namespace { : struct TServerStateInfo { : // The stringified version of the state. : string state; : : // A human-readable version of the time the tablet server was put into this : // state. : string time_entered_state; : : // The current status of the tserver (e.g. "Live", "Dead"). : string status; : : // A link to the tserver's web UI, if one exists. : string webserver; : }; : } // anonymous namespace I dont see this part being used in this file. Can we remove it? -- To view, visit http://gerrit.cloudera.org:8080/21823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f964c4f950edfde31772cafd5c3ed5d6b87413 Gerrit-Change-Number: 21823 Gerrit-PatchSet: 23 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 10 Jan 2025 11:56:59 +0000 Gerrit-HasComments: Yes
