Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: [WIP] KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 18: (8 comments) http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@125 PS17, Line 125: } // anonymous namespace > nit: add a few new lines (e.g. before if statements) for readability Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@136 PS17, Line 136: if (table_id.length() != 32) { : RETURN_JSON_ERROR(jw, "Invalid table ID", resp->status_code, HttpStatusCode::BadRequest); : } : CatalogManager::ScopedLeaderSharedLock l(master_->catalog_manager()); : scoped_refptr<TableInfo> table; : Status status = master_->catalog_manager()->GetTableInfo(table_id, &table); : : i > this kind of status check is present in almost every method introduced, ext Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@145 PS17, Line 145: } > could use macro/error handling function here as well, and all similar place Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@190 PS17, Line 190: > probably better to use your github username for TODOs, to make it more spec Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@214 PS17, Line 214: const string& json_str = req.post_data; > shouldn't the response status code be set to OK here? Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/master_path_handlers.cc@237 PS17, Line 237: > shouldn't the response status code be set to OK here? Done http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@135 PS17, Line 135: cluster_->mini_master()->bound_http_addr().ToString(), > probably should check the returned http status OK here ASSERT_OK checks whether the curl was successful or not. FetchURL returns Remote error if the status code is less than 200 or bigger or equal than 300, otherwise it returns OK. http://gerrit.cloudera.org:8080/#/c/21823/17/src/kudu/master/rest_catalog-test.cc@165 PS17, Line 165: K(CreateTestTable()); : string table_id = GetTableId("test_table"); : EasyCurl c; : faststring buf; : ASSERT_OK(c.FetchURL(Substitute("http://$0/api/v1/tables/$1";, : cluster_->mini_master()->bound_http_addr().ToString(), : table_id), : &buf)); > not sure if this schema is reused in the other tests, but if it is, then ex 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: 18 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, 09 Dec 2024 08:08:23 +0000 Gerrit-HasComments: Yes
