Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19890 )
Change subject: [tools] KUDU-1945: Kudu copy table tool ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/19890/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19890/3//COMMIT_MSG@7 PS3, Line 7: udu copy table tool While you are on this task, please make sure the 'kudu perf loadgen' CLI tool works as expected with auto-incrementing columns as well. If it does not, consider updating that as well in a separate changelist. http://gerrit.cloudera.org:8080/#/c/19890/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19890/2/src/kudu/tools/kudu-tool-test.cc@5737 PS2, Line 5737: kudu-tool-test.cc Is this a typo? http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/kudu-tool-test.cc@870 PS3, Line 870: // In kTestCopyTableSchemaOnly case, we may create table with RF=3, : // means 3 tservers needed at least. nit: update the comment correspondingly? BTW, is it crucial to have http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/kudu-tool-test.cc@1207 PS3, Line 1207: b.Build(schema); Should handle the return code? http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/table_scanner.cc File src/kudu/tools/table_scanner.cc: http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/table_scanner.cc@753 PS3, Line 753: // If we are copying a table we do not want to scan the auto-incrementing column. nit: adding the WHY part to this comment would be great http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/table_scanner.cc@882 PS3, Line 882: src_row.IsNull(j) I'm not sure the intermix of 'i' and 'j' results in correct outcode eventually given SetNull()/Set() is called at different indices. http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/table_scanner.cc@882 PS3, Line 882: dst_row->SetNull(j) Should handle the return Status code? http://gerrit.cloudera.org:8080/#/c/19890/3/src/kudu/tools/table_scanner.cc@883 PS3, Line 883: dst_row->Set ditto -- 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: 2 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: Mon, 15 May 2023 21:46:04 +0000 Gerrit-HasComments: Yes
