Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/21089 )
Change subject: [Tool] Return not OK status when copying tablets failed ...................................................................... Patch Set 4: (20 comments) http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@7 PS2, Line 7: OK > nit: OK Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@10 PS2, Line 10: non-OK > nit: non-OK Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@11 PS2, Line 11: it is not poss > nit: it's not possible to use Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: failed or > succeeded Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: > cases Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: message. : : This patch fixes this problem so now the CLI tool exits with non-OK : status > nit: you could omit/drop these details, but if you prefer to keep them, ple Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: 07c1c2a3bc568e20afc126b2 > nit: so now the CLI tool exits with non-OK status code Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: Ic9 > fixes Done http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: : > nit: 'some tablets failed to copy' or 'tablet copying failed for some table Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9544 PS2, Line 9544: InternalMiniClusterOptions opts; : opts.num_tablet_servers = 2; : NO_FATALS(StartMiniCluster(std::move(opts))); : NO_FATALS(CreateTableWithFlushedData("table1", mini_cluster_.get(), 3, 1)); > Is this possible to use all the default setting and just a single table to It needs to mock the case: copy a tablet from a tablet server to another tablet server. So 'opts.num_tablet_servers' should be 2. The default value of 'opts.num_tablet_servers' is 1. Line 9547 can be removed. http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9550 PS2, Line 9550: const > nit for here and below: could this be 'const auto ...'? Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9555 PS2, Line 9555: // An attempt to copy a non-existent tablet fails, and the r > What is this for? mini_cluster_->mini_tablet_server(1) must be shutdown when copying a tablet from another tablet server. Because it needs to open block manger. Or it emits an error: "IO error: failed to load instance files: Could not lock instance file. Make sure that Kudu is not already running and you are not trying to run Kudu with a different user than before." http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9557 PS2, Line 9557: > nit: OK Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9557 PS2, Line 9557: tus s = RunActionStderrString( > An attempt to copy a non-existent tablet fails Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9560 PS2, Line 9560: > nit: is this setting essential for the test scenario? If not, then don't a Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9561 PS2, Line 9561: ource_tserver_ > nit: to be more explicit, maybe use "non-existent-tablet-ids-str" or simila Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9565 PS2, Line 9565: ASSERT_STR_CONTAINS(s > Do we expect some particular non-OK status (e.g., Status::RuntimeError) her Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc@343 PS2, Line 343: std::atomi > Please use std::atomic<bool> instead: chromium-based Atomics are deprecated Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc@385 PS2, Line 385: !failed_tablet_ids.empty()) > nit: please instead use Done http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc@405 PS2, Line 405: some tablets failed to c > some tablets failed to copy: check error messages for details Done -- To view, visit http://gerrit.cloudera.org:8080/21089 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic957cbc379645e0607c1c2a3bc568e20afc126b2 Gerrit-Change-Number: 21089 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Comment-Date: Fri, 01 Mar 2024 04:10:57 +0000 Gerrit-HasComments: Yes
