Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22744 )
Change subject: KUDU-3608 REST API Follow-up ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog-test.cc@234 PS3, Line 234: ASSERT_TRUE(!s.ok()); Here and elsewhere: consider checking for exact type of non-OK status expected, e.g. here it might be ASSERT_TRUE(s.IsNotFound()) << s.ToString(); You could check other existing test scenarios for a reference. http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_path_handlers.cc@90 PS3, Line 90: if (req.path_params.count("table_id") > 0) { : table_id = req.path_params.at("table_id"); Consider using find() instead of double lookup. http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_path_handlers.cc@197 PS3, Line 197: Status status = master_->catalog_manager()->CreateTableWithUser(&request, &response, user); : : if (!status.ok()) { : RETURN_JSON_ERROR(jw, status.ToString(), *status_code, HttpStatusCode::InternalServerError); : } IIUC, CreateTableWithUser() may return Status::NotAuthorized() and Status::InvalidArgument(), where the former would naturally map into HTTP 403 and the latter into HTTP 400. Why are we converting all those client-side errors into the server-side InternalServerError? That doesn't look correct semantically and hinders proper status handling at the client side. Elsewhere, the situation with the conversion of Status into HTTP status codes looks quite similar. Maybe, there should be at least some generic mapping from Status into HTTP status codes? http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_test_base.h File src/kudu/master/rest_catalog_test_base.h: http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_test_base.h@54 PS3, Line 54: columnNames nit for here and elsewhere: the Kudu project doesn't use the "camelCase" for variables naming in C++ code, it rather uses the "snake_case" http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog_test_base.h@55 PS3, Line 55: columnNames.emplace_back("key"); nit: these two lines might have become just const std::vector<std::string column_names{ "key" }; -- To view, visit http://gerrit.cloudera.org:8080/22744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3cdb14d5a932eb8d1ba209ca803731749117024e Gerrit-Change-Number: 22744 Gerrit-PatchSet: 3 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: Sun, 13 Apr 2025 16:20:44 +0000 Gerrit-HasComments: Yes
