Abhishek Chennaka 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 Done http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@9 PS5, Line 9: kudu copy table tool > 'kudu table copy' CLI tool Done http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@10 PS5, Line 10: auto-incrmenting > auto-incrementing Done 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" Done http://gerrit.cloudera.org:8080/#/c/19890/5//COMMIT_MSG@16 PS5, Line 16: perf loadgen tool > 'kudu perf loadgen' CLI tool Done 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 Done 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 Done 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/defau Done http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3768 PS5, Line 3768: auto incrementing > auto-incrementing Done 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 ye Done http://gerrit.cloudera.org:8080/#/c/19890/5/src/kudu/tools/kudu-tool-test.cc@3781 PS5, Line 3781: date > data ? Done 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? Using RunTool() 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 Done 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 Done 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 In the copy task we scan the entire source table with all the columns. With the auto-incrementing column as a part of the primary key, it is always expected to be present in the scan result. But I guess adding an explicit check on the schemas of the two tables being equal wouldn't hurt. 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 Yes, thanks for pointing that out. -- 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 06:14:48 +0000 Gerrit-HasComments: Yes
