Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: [WIP] KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 25: (11 comments) Thank you for working on this! I took a quick look just to see if there is something high-level that I might recommend even at the WIP stage. Left a few suggestions. 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 the GROUP_FLAG_VALIDATOR macro is used in the existing code. http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc@259 PS25, Line 259: rest_catalog_path_handlers_ Do we need this if FLAGS_enable_rest_api = false? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/master.cc@305 PS25, Line 305: FLAGS_enable_rest_api Since this code is run just once during initialization, there isn't much sense trying to use the `--enable_rest_api` flag as a run-time one. With that, consider switching this criteria based on whether rest_catalog_path_handlers_ is nullptr or not. 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: Status CreateTestTable() { : string kTableName = "test_table"; : KuduSchema schema; : KuduSchemaBuilder b; : b.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull()->PrimaryKey(); : b.AddColumn("int_val")->Type(KuduColumnSchema::INT32)->NotNull(); : KUDU_CHECK_OK(b.Build(&schema)); : vector<string> columnNames; : columnNames.emplace_back("key"); : : // Set the schema and range partition columns. : KuduTableCreator* tableCreator = client_->NewTableCreator(); : tableCreator->table_name(kTableName).schema(&schema).set_range_partition_columns(columnNames); : : // Generate and add the range partition splits for the table. : int32_t increment = 1000 / 10; : for (int32_t i = 1; i < 10; i++) { : KuduPartialRow* row = schema.NewRow(); : KUDU_CHECK_OK(row->SetInt32(0, i * increment)); : tableCreator->add_range_partition_split(row); : } : tableCreator->num_replicas(1); : Status s = tableCreator->Create(); : delete tableCreator; : return s; : } : : string GetTableId(const string& table_name) { : shared_ptr<KuduTable> table; : Status s = client_->OpenTable(table_name, &table); : if (!s.ok()) { : LOG(ERROR) << "OpenTable failed: " << s.ToString(); : return ""; : } : if (table->name() == table_name) { : return table->id(); : } : return ""; : } This looks like copy-and-paste from spnego_rest_catalog-test.cc. Maybe, introduce a base test class or at least separate the common code as utility methods shared across multiple tests? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/rest_catalog-test.cc@132 PS25, Line 132: string column_id_str = std::to_string(column_id); : string column_id_1_str = std::to_string(column_id + 1); There is no need to convert integers to strings if using Substitute. 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: const vector<string> kExtraFlags = { : "--enable_rest_api" : }; Why to introduce this local constant if it's used only once at line 70? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@84 PS25, Line 84: KUDU_CHECK_OK Why KUDU_CHECK_OK? There is RETURN_NOT_OK() to handle this in a method returning Status. http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@89 PS25, Line 89: tableCreator Wrap this into std::unique_ptr? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/master/spnego_rest_catalog-test.cc@112 PS25, Line 112: if (table->name() == table_name) { : return table->id(); : } Why to add this extra check if the table was opened successfully? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS25: Separate this and other generic changes in web_callback_registry.h into its own changelist? http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/util/web_callback_registry.h File src/kudu/util/web_callback_registry.h: PS25: Separate this and other generic changes in webserver.cc into its own changelist? -- 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: 25 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: Wed, 12 Feb 2025 18:00:00 +0000 Gerrit-HasComments: Yes
