Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 30: (9 comments) http://gerrit.cloudera.org:8080/#/c/21823/29/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/29/src/kudu/master/rest_catalog_path_handlers.cc@210 PS29, Line 210: ; > Would it be worth to make this configurable? (flag) Please define a flag in this file for this: rest_catalog_default_request_timeout_ms = 30 * 1000; (i guess 30 sec should be enough here) it should have the tags: advanced, and runtime. http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@70 PS30, Line 70: if (!l.catalog_status().ok()) { : RETURN_JSON_ERROR( : jw, l.catalog_status().ToString(), resp->status_code, HttpStatusCode::ServiceUnavailable); : } : if (!l.leader_status().ok()) { : RETURN_JSON_ERROR( : jw, "Master is not the leader", resp->status_code, HttpStatusCode::ServiceUnavailable); : } since this comes up in multiple places i think its worth to create a function for this part: CheckIsInitializedAndIsLeader() (in the followup patch we can extend that function to CheckIsInitializedAndIsLeaderOrRedirect() or something like this) http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@95 PS30, Line 95: } else { : RETURN_JSON_ERROR( : jw, "Method not allowed", resp->status_code, HttpStatusCode::MethodNotAllowed); : } IMO it makes sense to do a check for the supported methods earlier. (if we get an unsupported method we dont even need to do all the catalog manager stuff) we can do this before the table id check. maybe its worth to create a function CheckSupportedRequestMethod() where you supply the webrequest, and the array of supported methods for that endpoint. http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@108 PS30, Line 108: if (!l.catalog_status().ok()) { : RETURN_JSON_ERROR( : jw, l.catalog_status().ToString(), resp->status_code, HttpStatusCode::ServiceUnavailable); : } : if (!l.leader_status().ok()) { : RETURN_JSON_ERROR( : jw, "Master is not the leader", resp->status_code, HttpStatusCode::ServiceUnavailable); : } same as above http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@119 PS30, Line 119: if (!l.catalog_status().ok()) { : RETURN_JSON_ERROR( : jw, l.catalog_status().ToString(), resp->status_code, HttpStatusCode::ServiceUnavailable); : } : if (!l.leader_status().ok()) { : RETURN_JSON_ERROR( : jw, "Master is not the leader", resp->status_code, HttpStatusCode::ServiceUnavailable); : } same as above http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@128 PS30, Line 128: else { : RETURN_JSON_ERROR( : jw, "Method not allowed", resp->status_code, HttpStatusCode::MethodNotAllowed); : } same as above http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@172 PS30, Line 172: google::protobuf::util::Status google_status nit: const auto s = ... http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@213 PS30, Line 213: google::protobuf::util::Status google_status nit: const auto s = ... http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_path_handlers.cc@237 PS30, Line 237: status = master_->catalog_manager()->IsAlterTableDone(&check_req, &check_resp, user); I guess since this logic is also available for create table, it would make sense to check also the create table operation whether it is done. -- 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: 30 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: Mon, 24 Mar 2025 13:06:40 +0000 Gerrit-HasComments: Yes
