Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19698 )
Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump ...................................................................... Patch Set 4: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2726 PS4, Line 2726: if (env_->IsEncryptionEnabled()) { : encryption_args = GetEncryptionArgs() + " --instance_file=" + : fs.GetInstanceMetadataPath(kTestDir); : } Is this a duplicate? If it's needed, would be great to add a comment to explain why. http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2736 PS4, Line 2736: "true", "1", "yes", "decoded" Aren't these the same as per https://kudu.apache.org/docs/command_line_tools_reference.html#wal-dump? If so, why have these logically duplicate values that's the same code path exercised? http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2743 PS4, Line 2743: ASSERT_STR_MATCHES(stdout, "Auto Incrementing Counter: 90"); : ASSERT_STR_MATCHES(stdout, "auto_incrementing_id=91"); : ASSERT_STR_MATCHES(stdout, "auto_incrementing_id=92"); Does it make sense to add a scenario to make sure these entries (without exact values) aren't printed out when running the tool with the same flags against a WAL segment without auto-incrementing column? Probably, modifying an existing test scenario will do as well. http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2750 PS4, Line 2750: "0", "no" Remove the logical dups? http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2757 PS4, Line 2757: ASSERT_STR_NOT_MATCHES(stdout, "Auto Incrementing Counter: 90"); : ASSERT_STR_NOT_MATCHES(stdout, "auto_incrementing_id=91"); : ASSERT_STR_NOT_MATCHES(stdout, "auto_incrementing_id=92"); nit for here and below: for these ASSERT_STR_NOT_MATCHES() it would make a stronger case if not specifying the exact counter values? -- To view, visit http://gerrit.cloudera.org:8080/19698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2 Gerrit-Change-Number: 19698 Gerrit-PatchSet: 4 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: Marton Greber <[email protected]> Gerrit-Comment-Date: Thu, 13 Apr 2023 05:06:15 +0000 Gerrit-HasComments: Yes
