Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: [WIP] KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 26: (9 comments) http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc@137 PS25, Line 137: The flag webserver_enabled must be " : "set to true for this flag to take effect. > Add a group flag validator to enforce this. For reference, check out how t Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc@259 PS25, Line 259: >set_member_type(resp.membe > Do we need this if FLAGS_enable_rest_api = false? Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc@305 PS25, Line 305: > Since this code is run just once during initialization, there isn't much se Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/rest_catalog-test.cc@82 PS25, Line 82: : static string ConstructTableSchema(int column_id, bool is_there_new_column = false) { : string columns = Substitute( : "{\"id\":$0,\"name\":\"key\",\"type\":\"INT32\",\"is_key\":" : "true,\"is_nullable\":false,\"encoding\":\"AUTO_ENCODING\",\"compression\":" : "\"DEFAULT_COMPRESSION\",\"cfile_block_size\":0,\"immutable\":false},{\"id\":" : "$1,\"name\":\"int_val\",\"type\":\"INT32\",\"is_key\":false,\"is_nullable\":" : "false,\"encoding\":\"AUTO_ENCODING\",\"compression\":\"DEFAULT_COMPRESSION\"," : "\"cfile_block_size\":0,\"immutable\":false}", : column_id, : column_id + 1); : if (is_there_new_column) { : string column_id_2_str = std::to_string(column_id + 2); : string new_column = Substitute( : ",{\"id\":$0,\"name\":\"new_column\",\"type\":\"STRING\",\"is_key\":false," : "\"is_nullable\":true,\"encoding\":\"AUTO_ENCODING\",\"compression\":" : "\"DEFAULT_COMPRESSION\",\"cfile_block_size\":0,\"immutable\":false}", : column_id_2_str); : columns += new_column; : } : return Substitute("{\"columns\":[$0]}", columns); : } : : protected: : unique_ptr<InternalMiniCluster> cluster_; : const std::string kTablePartitionSchema = "{\"range_schema\":{\"columns\":[{\"id\":10}]}}"; : const std::string kTablePartitionSchemaColumnIdZero = : "{\"range_schema\":{\"columns\":[{\"id\":0}]}}"; : }; : : : TEST_F(RestCatalogTest, TestInvalidMethod) { : EasyCurl c; : c.set_custom_method("DELETE"); : faststring buf; : Status s = c.FetchURL( : Substitute("http://$0/api/v1/tables";, cluster_->mini_master()->bound_http_addr().ToString()), : &buf); : A > This looks like copy-and-paste from spnego_rest_catalog-test.cc. Maybe, in Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/rest_catalog-test.cc@132 PS25, Line 132: table_id), : &buf); > There is no need to convert integers to strings if using Substitute. Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@65 PS25, Line 65: ASSERT_OK(StartCluster()); : ASSERT_OK(cluster_->kdc()->Kinit("test-admin")); : ASSE > Why to introduce this local constant if it's used only once at line 70? Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@84 PS25, Line 84: > Why KUDU_CHECK_OK? There is RETURN_NOT_OK() to handle this in a method ret Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@89 PS25, Line 89: > Wrap this into std::unique_ptr? Done http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@112 PS25, Line 112: : : > Why to add this extra check if the table was opened successfully? 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: 26 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, 03 Mar 2025 12:57:54 +0000 Gerrit-HasComments: Yes
