Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions ...................................................................... Patch Set 6: Code-Review+1 (4 comments) Just a couple of nits, and I think we are good. Thank you for working on this! http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc File src/kudu/master/catalog_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@277 PS6, Line 277: CatalogManagerTest Please rename the test class to reflect whats the purpose of this test. Something like this would do the job: CatalogManagerRpcAndUserFunctionsTest http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316 PS6, Line 316: CreateTableTest Please rename this function as is can be confusing to have a CreateTestTable and a CreateTableTest function. This function could be rename to something like: PopulateCreateTableRequest. http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager-test.cc@316 PS6, Line 316: CreateTableResponsePB& resp if not used in the function, you can remove this argument. http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/22420/6/src/kudu/master/catalog_manager.cc@2233 PS6, Line 2233: if (!rpc) { : LOG(INFO) << "rpc is nullptr, calling CreateTableHelper with nullptr."; : return CreateTableHelper(orig_req, resp, nullptr, std::nullopt); : } : : return CreateTableHelper(orig_req, resp, rpc, std::nullopt); nit: i think we dont need the if check there, right? we can just simply call: return CreateTableHelper(orig_req, resp, rpc, std::nullopt); In the helper function where rpc is used we have proper if checks there. -- To view, visit http://gerrit.cloudera.org:8080/22420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5 Gerrit-Change-Number: 22420 Gerrit-PatchSet: 6 Gerrit-Owner: Gabriella Lotz <[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, 10 Feb 2025 18:56:51 +0000 Gerrit-HasComments: Yes
