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 32: (11 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: > do we need this line? Done http://gerrit.cloudera.org:8080/#/c/21823/30/src/kudu/master/rest_catalog-test.cc@211 PS30, Line 211: EasyCurl c; > do we need this line? Done 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: int first_column_id = FindColumnId(buf.ToString()); : ASSERT_NE(first_column_id, -1) << "Column ID not found in the schema"; > nit: can you please add a small comment here? Done http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog-test.cc@256 PS31, Line 256: faststring buf; > nit: maybe assert the error message? Curious if we return reasonable error I checked and it is empty. 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@66 PS31, Line 66: CheckIsInitializedAndIsLeader(Json > nit rename: CheckIsInitializedAndIsLeader Done http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_path_handlers.cc@86 PS31, Line 86: RETURN_JSON_ERROR(jw, > nit: maybe add more error message detail? Done http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_path_handlers.cc@159 PS31, Line 159: atus_code = HttpStatusCode::Ok; > does it make sense to only create one JsonWriter instance in the beginning Done http://gerrit.cloudera.org:8080/#/c/21823/31/src/kudu/master/rest_catalog_path_handlers.cc@229 PS31, Line 229: > does it make sense to only create one JsonWriter instance in the beginning Done 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: DCHECK(table_id); : kudu::client::sp::shared_ptr<kudu::client::KuduTable> table; : RETURN_NOT_OK(client_->OpenTable(table_name, &table)); : *table_id = table->id(); : return Status::OK(); : } : : kudu::client::sp::shared_ptr<kudu::client::KuduClient> client_; : s > please refactor this to return a status, and provide the table id as an out Done 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: kudu::client::KuduSchema schema; > make this a class member field, such that else in the tests you dont have t Done 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: ASSERT_OK(client_->OpenTable(kTableName, &table)); : ASSERT_EQ(table->schema().num_columns(), 3); : ASSERT_STR_CONTAINS(table->owner(), "alice"); : } : : TEST_F(SpnegoRestCatalogTest, TestInvalidHeaders) { : ASSERT_OK(CreateTestTable()); : string table_id; : ASSERT_OK(GetTableId(kTableName, &table_id)); : 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";, cluster_->mini_master()->bound_http_addr().ToString()), : &buf, : {"Authorization: Negotiate I am also an invalid header"}); : > IMO is enough to use one endpoint and have one HTTP 500 and one HTTP 401 te 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: 32 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: Tue, 25 Mar 2025 21:03:45 +0000 Gerrit-HasComments: Yes
