Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19594 )
Change subject: [KUDU-3452] Make validate table creating task not affected ...................................................................... Patch Set 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/master/catalog_manager.cc@5971 PS13, Line 5971: , nit: should be ", " http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9411 PS13, Line 9411: ASSERT_STR_CONTAINS(s.ToString() nit: Also check the Status type is Timeout? http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9402 PS13, Line 9402: { : unique_ptr<client::KuduTableCreator> table_creator( : client->NewTableCreator()); : Status s = table_creator->table_name(kNotEnoughTServersTableId) : .schema(&schema) : .set_range_partition_columns({ "key" }) : .num_replicas(cluster_->num_tablet_servers()) : .timeout(MonoDelta::FromMilliseconds(kTimeout)) : .Create(); : ASSERT_STR_CONTAINS(s.ToString(), "Timed out waiting for Table Creation"); : } There are some duplicate code, how about encapsulate a function? http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9442 PS13, Line 9442: ASSERT_EQ(2 Better to check the 2 tables are exactly kTwoReplicasTableId and kOneReplicasTableId? http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9445 PS13, Line 9445: if (items[i]["name"].GetString() == kTwoReplicasTableId) { : ASSERT_EQ(string("HEALTHY"), items[i]["health"].GetString()); : } : if (items[i]["name"].GetString() == kOneReplicasTableId) { : ASSERT_EQ(string("HEALTHY"), items[i]["health"].GetString()); : } nit: how about? if (items[i]["name"].GetString() == kTwoReplicasTableId || items[i]["name"].GetString() == kOneReplicasTableId) { ASSERT_EQ(string("HEALTHY"), items[i]["health"].GetString()); } http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9457 PS13, Line 9457: string out; : ASSERT_EVENTUALLY([&] { : NO_FATALS(RunActionStdoutString( : Substitute("tserver list $0 --columns=uuid,state --format=csv", master_address), &out)); : ASSERT_STR_CONTAINS(out, Substitute("$0,$1", stopped_ts_uuid, "NONE")); : }); I'm not sure why check the tserver and the cluster state? It seems not meaningful for this test, this test only check that tables can be created even though a previous table created in progress. http://gerrit.cloudera.org:8080/#/c/19594/13/src/kudu/tools/kudu-tool-test.cc@9463 PS13, Line 9463: ClusterVerifier cv(cluster_.get()); Also to check the status of table kNotEnoughTServersTableId is health now? -- To view, visit http://gerrit.cloudera.org:8080/19594 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I64668651d0e8f58b92cfb841bdb20617de6776f9 Gerrit-Change-Number: 19594 Gerrit-PatchSet: 13 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sun, 07 May 2023 16:05:55 +0000 Gerrit-HasComments: Yes
