Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19890 )
Change subject: [tools] KUDU-1945: Kudu copy table and per loadgen ...................................................................... Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@7 PS5, Line 7: Kudu copy table and per loadgen update 'table copy' and 'perf loadgen' CLI tools ? http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@9 PS5, Line 9: kudu copy table tool 'kudu table copy' CLI tool or 'table copy' CLI tool http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@10 PS5, Line 10: auto-incrmenting auto-incrementing http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@13 PS5, Line 13: during : each row write nit: "during each row's write" or "when writing each row" http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@16 PS5, Line 16: perf loadgen tool 'kudu perf loadgen' CLI tool or 'perf loadgen' CLI tool http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@17 PS5, Line 17: . Please add a sentence to explicitly state that the 'kudu perf loadgen' CLI tool isn't yet working as expected with tables having auto-incrementing columns if using the --use_upsert option. http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3762 PS5, Line 3762: shared_ptr<KuduClient> client; : KuduSchema schema; nit: it's C++, so it's possible to keep the declaration of variables closer to the place they are used http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3765 PS5, Line 3765: ExternalMiniClusterOptions opts; : NO_FATALS(StartExternalMiniCluster(std::move(opts))); nit: StartExternalMiniCluster() have a parameter by default (an empty/default opts), so there is not need to do dance around empty/default ExternalMiniClusterOptions http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3768 PS5, Line 3768: auto incrementing auto-incrementing http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3777 PS5, Line 3777: .set_range_partition_columns({}) Is it crucial to have a non-partitioned table in this test scenario? If yes, please add a comment explaining why. http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3781 PS5, Line 3781: date data ? http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3783 PS5, Line 3783: RunActionStdoutString If stdout isn't used, maybe use RunActionStdoutNone() instead? http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3785 PS5, Line 3785: 100 nit: could add a constant and use it throughout the scenario http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3797 PS5, Line 3797: lines.size(), 101) nit: the expected value comes first http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/table_scanner.cc@881 PS5, Line 881: for (int dst_iterator = 0; dst_iterator < table->schema().num_columns(); dst_iterator++) { : if (auto_incrementing_col_idx != dst_iterator) { Is it guaranteed that we have column_ids normalized between the source and the destination table, so we can rely on the equality of ids for auto-incrementing columns? If so, please add a comment to explain why. http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/tool_action_perf.cc File src/kudu/tools/tool_action_perf.cc: http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/tool_action_perf.cc@534 PS5, Line 534: if (columns[idx].is_auto_incrementing()) { : continue; : } Shouldn't this be placed before the call to the value generator at line 532 to avoid calling the generator needlessly? Otherwise, if it's placed after that call intentionally, please add a comment explaining why. -- To view, visit http://gerrit.cloudera.org:8080/19890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I754a7e84c16d1f3b2d52be937e1eb50b3d00d759 Gerrit-Change-Number: 19890 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 23 May 2023 03:07:58 +0000 Gerrit-HasComments: Yes
