Alexey Serbin 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 2: (20 comments) Thank you for the fix! Just some nits, but overall looks good to me. 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 http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@10 PS2, Line 10: non-ok nit: non-OK http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@11 PS2, Line 11: it can not use nit: it's not possible to use http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: case cases http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: successful succeeded http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@12 PS2, Line 12: In some case, : using a Python script to call 'local_replica copy_from_remote' : command to copy tablets. Python script can not known it is failed : or not. nit: you could omit/drop these details, but if you prefer to keep them, please re-phrase these two sentences to be correct English http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: fix fixes http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: and return a non-ok status nit: so now the CLI tool exits with non-OK status code http://gerrit.cloudera.org:8080/#/c/21089/2//COMMIT_MSG@17 PS2, Line 17: some : tablets copying failed. nit: 'some tablets failed to copy' or 'tablet copying failed for some tablets' 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 implement this test scenario? http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9550 PS2, Line 9550: string nit for here and below: could this be 'const auto ...'? http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9555 PS2, Line 9555: NO_FATALS(mini_cluster_->mini_tablet_server(1)->Shutdown()); What is this for? http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9557 PS2, Line 9557: Copy a non-exist tablet will fail An attempt to copy a non-existent tablet fails http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9557 PS2, Line 9557: ok nit: OK http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9560 PS2, Line 9560: -num_threads=3 nit: is this setting essential for the test scenario? If not, then don't add it into the command line? http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9561 PS2, Line 9561: tablet_ids_str nit: to be more explicit, maybe use "non-existent-tablet-ids-str" or similar http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/kudu-tool-test.cc@9565 PS2, Line 9565: ASSERT_FALSE(s.ok()); Do we expect some particular non-OK status (e.g., Status::RuntimeError) here? If yes, maybe add an assertion on that. Also, is there anything particular to expect in the error string? If yes, maybe add an assertion on that as well. 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: AtomicBool Please use std::atomic<bool> instead: chromium-based Atomics are deprecated and should be removed from the codebase in the future. http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc@385 PS2, Line 385: failed_tablet_ids.size() > 0 nit: please instead use !failed_tablet_ids.empty() http://gerrit.cloudera.org:8080/#/c/21089/2/src/kudu/tools/tool_action_local_replica.cc@405 PS2, Line 405: Some tablets copy failed some tablets failed to copy: check error messages for details -- 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: 2 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 29 Feb 2024 19:42:10 +0000 Gerrit-HasComments: Yes
