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 31: (8 comments) http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog-test.cc@198 PS30, Line 198: c.set_verbose(true); do we need this line? http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog-test.cc@211 PS30, Line 211: c.set_verbose(true); do we need this line? http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog-test.cc@173 PS31, Line 173: string partition_schema = : (first_column_id == 0) ? kTablePartitionSchemaColumnIdZero : kTablePartitionSchema; nit: can you please add a small comment here? In DEBUG builds, column id starts from 10 while in RELEASE builds it starts from 0. http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog-test.cc@256 PS31, Line 256: ASSERT_STR_CONTAINS(s.ToString(), "HTTP 411"); nit: maybe assert the error message? Curious if we return reasonable error message in this case. http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_path_handlers.cc@86 PS31, Line 86: RETURN_JSON_ERROR(jw, "Invalid table ID", resp->status_code, HttpStatusCode::BadRequest); nit: maybe add more error message detail? "Invalid table ID: must be exactly 32 characters long." http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_test_base.h File src/kudu/master/rest_catalog_test_base.h: http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_test_base.h@79 PS31, Line 79: std::string GetTableId(const std::string& table_name) { : kudu::client::sp::shared_ptr<kudu::client::KuduTable> table; : kudu::Status s = client_->OpenTable(table_name, &table); : if (!s.ok()) { : LOG(ERROR) << "OpenTable failed: " << s.ToString(); : return ""; : } : return table->id(); : } please refactor this to return a status, and provide the table id as an out-parameter. http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_test_base.h File src/kudu/master/rest_catalog_test_base.h: http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog_test_base.h@51 PS30, Line 51: std::string kTableName = "test_table"; make this a class member field, such that else in the tests you dont have to refer to this table by typing "test_table" rather than kTableName http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/spnego_rest_catalog-test.cc@205 PS31, Line 205: TEST_F(SpnegoRestCatalogTest, TestInvalidHeaders) { : ASSERT_OK(CreateTestTable()); : string table_id = GetTableId("test_table"); : EasyCurl c; : faststring buf; : Status s = c.FetchURL( : Substitute("http://$0/api/v1/tables";, cluster_->mini_master()->bound_http_addr().ToString()), : &buf, : {"Authorization: I am an invalid header"}); : ASSERT_STR_CONTAINS(s.ToString(), "HTTP 500"); : s = c.FetchURL(Substitute("http://$0/api/v1/tables/$1";, : cluster_->mini_master()->bound_http_addr().ToString(), : table_id), : &buf, : {"Authorization: I am an invalid header"}); : ASSERT_STR_CONTAINS(s.ToString(), "HTTP 500"); : s = c.FetchURL( : Substitute("http://$0/api/v1/tables";, cluster_->mini_master()->bound_http_addr().ToString()), : &buf, : {"Authorization: Negotiate I am also an invalid header"}); : ASSERT_STR_CONTAINS(s.ToString(), "HTTP 401"); : } IMO is enough to use one endpoint and have one HTTP 500 and one HTTP 401 test in there. -- 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: 31 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 17:09:56 +0000 Gerrit-HasComments: Yes
