You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org> on 2023/04/06 01:12:01 UTC

[kudu-CR] [tools] KUDU-1945 Fix kudu wal dump

Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19698


Change subject: [tools] KUDU-1945 Fix kudu wal dump
......................................................................

[tools] KUDU-1945 Fix kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT replicate op and also populates the
corresponding auto incrementing column values for each row when printing
rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 155 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/1
-- 
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: newchange
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+2


-- 
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: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 18 Apr 2023 22:44:46 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Thu, 13 Apr 2023 05:06:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka 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:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19698/3/src/kudu/common/wire_protocol-test-util.h
File src/kudu/common/wire_protocol-test-util.h:

http://gerrit.cloudera.org:8080/#/c/19698/3/src/kudu/common/wire_protocol-test-util.h@101
PS3, Line 101: }
> Many tests fail with PS3.  Is that because of this change?
Yes, this is not where the new row needs to be added. Fixed it.



-- 
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 <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Wed, 12 Apr 2023 22:48:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19698

to look at the new patch set (#4).

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT replicate op and also populates the
corresponding auto incrementing column values for each row when printing
rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 167 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/4
-- 
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: newpatchset
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 6: Verified+1

Unrelated test failure in LogRollingITest.TestLogCleanupOnStartup


-- 
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: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 18 Apr 2023 22:43:52 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19698

to look at the new patch set (#6).

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT/INSERT_IGNORE replicate op and also populates
the corresponding auto incrementing column values for each row when
printing rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 171 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/6
-- 
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: newpatchset
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h
File src/kudu/common/wire_protocol-test-util.h:

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@25
PS2, Line 25: #include <kudu/client/schema.h>
This should moved into the section of local included below.


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@39
PS2, Line 39: inline client::KuduSchema GetAutoIncrementingTestSchema() {
nit: add an empty line to separate this definition of the new function from the function definition above


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@40
PS2, Line 40: kSchema
nit: why this fancy naming for a local variable?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@45
PS2, Line 45: b.Build(&kSchema);
This returns Status, so it should be handled appropriately.  Maybe, wrap this into CHECK_OK() at least?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2688
PS2, Line 2688:         
nit: wrong indent?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2691
PS2, Line 2691: 0, // schema_version
nit: could use the same notation for commenting on the parameter names as in other lines for this call site?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2702
PS2, Line 2702: 0
Could we use some other non-zero number to be able to spot some issues with non-properly initialized proto fields elsewhere (e.g., 0x5a)?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2704
PS2, Line 2704:    AddTestRowToPB(RowOperationsPB::INSERT, schema,
              :                    opid.index(),
              :                    0,
              :                    "this is a test insert",
              :                    write->mutable_row_operations());
Could we have a case where at least two rows are written, so we could see that the auto-incrementing counter comes out incrementing, indeed?


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2805
PS2, Line 2805: 
nit: remove the extra empty line


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/tool_action_common.cc@351
PS2, Line 351:   if (write.has_auto_incrementing_column()) {
             :     int64_t auto_incrementing_counter =
             :         write.auto_incrementing_column().auto_incrementing_counter();
             :     RETURN_NOT_OK(dec.DecodeOperations<DecoderMode::WRITE_OPS>(&ops, &auto_incrementing_counter));
Why do we need this if auto_incrementing_counter value is discarded after exiting out of the if(){} scope?



-- 
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: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Fri, 07 Apr 2023 02:34:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19698/3/src/kudu/common/wire_protocol-test-util.h
File src/kudu/common/wire_protocol-test-util.h:

http://gerrit.cloudera.org:8080/#/c/19698/3/src/kudu/common/wire_protocol-test-util.h@101
PS3, Line 101:   AddTestRowWithNullableStringToPB(op_type, schema, key, int_val + 1, string_val.c_str(), ops);
Many tests fail with PS3.  Is that because of this change?



-- 
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: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Wed, 12 Apr 2023 16:03:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19698

to look at the new patch set (#5).

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT replicate op and also populates the
corresponding auto incrementing column values for each row when printing
rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 169 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/5
-- 
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: newpatchset
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
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: deleteVote
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19698/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19698/5//COMMIT_MSG@11
PS5, Line 11: INSERT
nit: INSERT/INSERT_IGNORE



-- 
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: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 18 Apr 2023 02:14:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19698

to look at the new patch set (#2).

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT replicate op and also populates the
corresponding auto incrementing column values for each row when printing
rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 156 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/2
-- 
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: newpatchset
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka 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 5:

(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: }
              : 
              : TEST_F(ToolTest, TestWalDumpWithAutoIncrementingColumn) {
              :   c
> Is this a duplicate?  If it's needed, would be great to add a comment to ex
It's duplicate. Removed it now.


http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2736
PS4, Line 2736: ayout());
> Aren't these the same as per https://kudu.apache.org/docs/command_line_tool
Removed the duplicate values.


http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2743
PS4, Line 2743:                         /*file_cache*/nullptr,
              :                         kTestTablet,
              :                         schema_with_id,
> Does it make sense to add a scenario to make sure these entries (without ex
Done


http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2750
PS4, Line 2750: 
> Remove the logical dups?
Done


http://gerrit.cloudera.org:8080/#/c/19698/4/src/kudu/tools/kudu-tool-test.cc@2757
PS4, Line 2757:     write->mutable_auto_incrementing_column()->set_auto_incrementing_counter(0x5a);
              :     ASSERT_OK(SchemaToPB(schema, write->mutable_schema()));
              :     AddTestRowToPB(RowOperationsPB::INSERT, schema,
> nit for here and below: for these ASSERT_STR_NOT_MATCHES() it would make a 
Done



-- 
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: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Mon, 17 Apr 2023 19:05:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19698

to look at the new patch set (#3).

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT replicate op and also populates the
corresponding auto incrementing column values for each row when printing
rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 164 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/19698/3
-- 
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: newpatchset
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka 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 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h
File src/kudu/common/wire_protocol-test-util.h:

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@25
PS2, Line 25: #include <kudu/client/schema.h>
> This should moved into the section of local included below.
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@39
PS2, Line 39: inline client::KuduSchema GetAutoIncrementingTestSchema() {
> nit: add an empty line to separate this definition of the new function from
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@40
PS2, Line 40: kSchema
> nit: why this fancy naming for a local variable?
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/common/wire_protocol-test-util.h@45
PS2, Line 45: b.Build(&kSchema);
> This returns Status, so it should be handled appropriately.  Maybe, wrap th
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2688
PS2, Line 2688:         
> nit: wrong indent?
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2691
PS2, Line 2691: 0, // schema_version
> nit: could use the same notation for commenting on the parameter names as i
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2702
PS2, Line 2702: 0
> Could we use some other non-zero number to be able to spot some issues with
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2704
PS2, Line 2704:    AddTestRowToPB(RowOperationsPB::INSERT, schema,
              :                    opid.index(),
              :                    0,
              :                    "this is a test insert",
              :                    write->mutable_row_operations());
> Could we have a case where at least two rows are written, so we could see t
Done


http://gerrit.cloudera.org:8080/#/c/19698/2/src/kudu/tools/kudu-tool-test.cc@2805
PS2, Line 2805: 
> nit: remove the extra empty line
Done



-- 
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: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 21:51:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19698/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/19698/5/src/kudu/tools/kudu-tool-test.cc@2842
PS5, Line 2842: : 
nit: probably, it makes sense to keep this as in other lines with not matching "Auto Incrementing Counter" pattern (e.g., lines 2806, 2818, 2830)


http://gerrit.cloudera.org:8080/#/c/19698/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:

http://gerrit.cloudera.org:8080/#/c/19698/5/src/kudu/tools/tool_action_common.cc@352
PS5, Line 352:     int64_t auto_incrementing_counter =
readability nit: could you add a comment that `auto_incrementing_counter` is used as in-out argument for DecodeOperations() call below?



-- 
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: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Tue, 18 Apr 2023 02:10:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19698 )

Change subject: [tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump
......................................................................

[tools] KUDU-1945 Print auto-incrementing counter in kudu wal dump

This patch allows the "kudu wal dump" tool to display auto-incrementing
counter value if present in the WAL segment. It displays the counter
value present in each INSERT/INSERT_IGNORE replicate op and also populates
the corresponding auto incrementing column values for each row when
printing rows.

Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Reviewed-on: http://gerrit.cloudera.org:8080/19698
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
3 files changed, 171 insertions(+), 2 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I4e807aaef48683ec7c5317eecdedf8e6e15950e2
Gerrit-Change-Number: 19698
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>