Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19571 )
Change subject: KUDU-3452 Create tablet without enough healthy tservers ...................................................................... Patch Set 15: (10 comments) http://gerrit.cloudera.org:8080/#/c/19571/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19571/15//COMMIT_MSG@7 PS15, Line 7: enough What does "enough" mean, does 1 out of RF = 3 is enough? How about to describe "all requested replicas number of" or sth. like that? http://gerrit.cloudera.org:8080/#/c/19571/15//COMMIT_MSG@10 PS15, Line 10: enough Same. http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9205 PS15, Line 9205: opts.extra_master_flags.emplace_back("--catalog_manager_check_ts_count_for_create_table=false"); nit: they can be simplified by: cluster_opts_.extra_master_flags = { "--catalog_manager_check_ts_count_for_alter_table=false", Substitute("--tserver_unresponsive_timeout_ms=$0", kTSUnresponsiveTimeoutMs)), ... } http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9237 PS15, Line 9237: ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); nit: because many place would return InvalidArgument, so also check the error meesage is as expect, it contains some key words like "not enough registered tablet servers to create a table with the requested replication" http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9239 PS15, Line 9239: RunTool(Substitute("table list $0 --tables=$1", master_addr, kOverRegistedTSTableName), How about use client to OpenTable and then check the return status is NotFound? CLI tool is too heavy. http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9244 PS15, Line 9244: // Ksck will be OK. Why check ksck? http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9262 PS15, Line 9262: shared_ptr<KuduClient> client; The 2nd time this logic appears, how about encapsulate it as a function? http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9277 PS15, Line 9277: ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); Same, check the error meesage contains some key words. http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9309 PS15, Line 9309: NO_FATALS(workload.Start()); The TestWorkload will start write threads to write data into the table, I don't think it is what you need, right? How about using the encapsulate function above? http://gerrit.cloudera.org:8080/#/c/19571/15/src/kudu/tools/kudu-tool-test.cc@9312 PS15, Line 9312: insert some data Is inserting data necessary? If the table status is health, it's not needed to do that. -- To view, visit http://gerrit.cloudera.org:8080/19571 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I742ba1ff770f5c8b1be5800334c29bec96e195c6 Gerrit-Change-Number: 19571 Gerrit-PatchSet: 15 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) 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: Sat, 06 May 2023 07:42:50 +0000 Gerrit-HasComments: Yes
