Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 34: (23 comments) Thank you for all the comments! I created a follow-up for these changes, that can be found here: https://gerrit.cloudera.org/c/22744/ http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog-test.cc@537 PS34, Line 537: s = client_->OpenTable(kTableName, &table); : ASSERT_TRUE(s.ok()); > nit for here and elsewhere where the status isn't used elsewhere: it might Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h File src/kudu/master/rest_catalog_path_handlers.h: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@34 PS34, Line 34: class RestCatalogPathHandlers { > nit: consider making this class 'final' Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@36 PS34, Line 36: master > nit: consider adding DCHECK_NOT_NULL wrapper around 'master' Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@38 PS34, Line 38: ~RestCatalogPathHandlers(); > nit: if the destructor is empty, consider using Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@24 PS34, Line 24: #include "google/protobuf/util/json_util.h" > nit: change quotes to '<' and '>' and re-arrange the lines around to be sor Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@71 PS34, Line 71: JsonWriter* jw > Why is this passed by pointer if 'jw' isn't changing in this method? Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@73 PS34, Line 73: HttpStatusCode* status_code > Why is this passed by pointer if 'status_code' isn't changing in this metho Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@90 PS34, Line 90: at("table_id") > What if 'table_id' isn't there? There will be an exception thrown, and the Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@108 PS34, Line 108: HttpStatusCode::ServiceUnavailable > For here and elsewhere: why not to return InternalServerError or other serv Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@146 PS34, Line 146: std:: > nit for here and elsewhere: remove the 'std::' prefix since there are corre Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@322 PS34, Line 322: Status status = master_->catalog_manager()->GetTableInfo(table_id, &table); > What if status is non-OK here? Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@346 PS34, Line 346: Status > If the method doesn't return anything but Status::OK(), maybe it should hav Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h File src/kudu/master/rest_catalog_test_base.h: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@43 PS34, Line 43: DECLARE_bool(enable_rest_api); > What is this for? Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@50 PS34, Line 50: kudu:: > nit for here and elsewhere: I'd think of removing the 'kudu::' namespace pr Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@64 PS34, Line 64: int32_t increment > nit: this could be constexpr Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@74 PS34, Line 74: kudu::Status s = tableCreator->Create(); : return s; > return tableCreator->Create(); Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@67 PS34, Line 67: "alice" > nit for here and elsewhere in this test: introduce a const/constexpr static Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@72 PS34, Line 72: auto opts = InternalMiniClusterOptions(); > nit: in C++ that's usually expressed as Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@75 PS34, Line 75: opts > nit: wrap this into std::move() to avoid extra copying Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@147 PS34, Line 147: ASSERT_STR_CONTAINS(s.ToString(), "OK"); > This isn't a reliable way to check for OK status because error messages can Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@206 PS34, Line 206: ASSERT_EQ(table->schema().num_columns(), 3); > nit for here and elsewhere: the expected/reference value should come first Done http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@220 PS34, Line 220: ASSERT_STR_CONTAINS(s.ToString(), "HTTP 500"); > Here and elsewhere in case of non-OK status: add an assertion for the exact Although specific handling is not yet implemented for all error types, I have included an extra check to ensure that the status s is not OK. http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@229 PS34, Line 229: #ifndef __APPLE__ > nit: please add a small text blurb to explain why this isn't enabled for ma 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: 34 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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Mon, 07 Apr 2025 12:00:59 +0000 Gerrit-HasComments: Yes
