You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2021/08/18 18:40:23 UTC
[kudu-CR] [fuzz-itest] add WAL GCs
Hello Alexey Serbin, Bankim Bhavsar,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/17787
to review the following change.
Change subject: [fuzz-itest] add WAL GCs
......................................................................
[fuzz-itest] add WAL GCs
In the past we've had bugs that only surface when WALs were GCed.
Additionally, implementing certain changes to the tablet key-value store
typically must also account for being able to correctly bootstrap from
the WALs. To facillitate testing such changes, this patch adds a WAL GC
op to the fuzz test.
I had to add some error handling to allow writes to collide with the
destruction of the server.
Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/fuzz-itest.cc
2 files changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/17787/1
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17787
to look at the new patch set (#4).
Change subject: [fuzz-itest] add WAL GCs
......................................................................
[fuzz-itest] add WAL GCs
In the past we've had bugs that only surface when WALs were GCed.
Additionally, implementing certain changes to the tablet key-value store
typically must also account for being able to correctly bootstrap from
the WALs. To facilitate testing such changes, this patch adds a WAL GC
op to the fuzz test.
This also adds a bytes equivalent of the --log_segment_size_mb flag, to
speed up the generation of WAL segments.
I had to add some error handling to allow writes to collide with the
destruction of the server.
Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/fuzz-itest.cc
3 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/17787/4
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
[fuzz-itest] add WAL GCs
In the past we've had bugs that only surface when WALs were GCed.
Additionally, implementing certain changes to the tablet key-value store
typically must also account for being able to correctly bootstrap from
the WALs. To facilitate testing such changes, this patch adds a WAL GC
op to the fuzz test.
This also adds a bytes equivalent of the --log_segment_size_mb flag, to
speed up the generation of WAL segments.
I had to add some error handling to allow writes to collide with the
destruction of the server.
Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Reviewed-on: http://gerrit.cloudera.org:8080/17787
Tested-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/fuzz-itest.cc
3 files changed, 36 insertions(+), 2 deletions(-)
Approvals:
Andrew Wong: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17787
to look at the new patch set (#2).
Change subject: [fuzz-itest] add WAL GCs
......................................................................
[fuzz-itest] add WAL GCs
In the past we've had bugs that only surface when WALs were GCed.
Additionally, implementing certain changes to the tablet key-value store
typically must also account for being able to correctly bootstrap from
the WALs. To facilitate testing such changes, this patch adds a WAL GC
op to the fuzz test.
I had to add some error handling to allow writes to collide with the
destruction of the server.
Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/fuzz-itest.cc
2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/17787/2
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17787
to look at the new patch set (#3).
Change subject: [fuzz-itest] add WAL GCs
......................................................................
[fuzz-itest] add WAL GCs
In the past we've had bugs that only surface when WALs were GCed.
Additionally, implementing certain changes to the tablet key-value store
typically must also account for being able to correctly bootstrap from
the WALs. To facilitate testing such changes, this patch adds a WAL GC
op to the fuzz test.
This also adds a bytes equivalent of the --log_segment_size_mb flag, to
speed up the generation of WAL segments.
I had to add some error handling to allow writes to collide with the
destruction of the server.
Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/fuzz-itest.cc
3 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/17787/3
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17787/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/17787/4/src/kudu/consensus/log.cc@146
PS4, Line 146: og_segment_size_bytes_for_tests
> For test only flags we should make them hidden.
From util/flag_tags.h
// - "unsafe":
// These flags are for internal use only (e.g. testing), and changing
// them away from the defaults may result in arbitrarily bad things
// happening. These flags are automatically excluded from user-facing
// documentation even if they are not also marked 'hidden'.
//
// Users must pass --unlock_unsafe_flags to use any of these
// flags.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Aug 2021 02:03:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc@374
PS1, Line 374: // NOTE: we only publicly expose a means to set the segment size in
> Added a comment explaining.
Actually it seems like this isn't threadsafe. I'll add a test-only flag for bytes instead of MiB.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 18 Aug 2021 21:50:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17787/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17787/1//COMMIT_MSG@12
PS1, Line 12: facillitate
nit: facilitate
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc@374
PS1, Line 374: replicas[0]->log()->SetMaxSegmentSizeForTests(kLowSegmentSizeBytes);
Why not to do so as a part of CreateTabletAndStartClusterWithSchema() ?
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 18 Aug 2021 19:15:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc@374
PS1, Line 374: CHECK_EQ(1, replicas.size());
> Actually it seems like this isn't threadsafe. I'll add a test-only flag for
Yep, I guess a test-only flag is a more straightforward path to go.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Aug 2021 02:37:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 1: Code-Review+2
Good addition!
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 18 Aug 2021 18:48:55 +0000
Gerrit-HasComments: No
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17787/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/17787/4/src/kudu/consensus/log.cc@146
PS4, Line 146: og_segment_size_bytes_for_tests
For test only flags we should make them hidden.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Aug 2021 01:31:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 4: Verified+1
Test failure is unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Aug 2021 00:02:58 +0000
Gerrit-HasComments: No
[kudu-CR] [fuzz-itest] add WAL GCs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17787 )
Change subject: [fuzz-itest] add WAL GCs
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17787/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17787/1//COMMIT_MSG@12
PS1, Line 12: facilitate
> nit: facilitate
Done
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17787/1/src/kudu/integration-tests/fuzz-itest.cc@374
PS1, Line 374: // NOTE: we only publicly expose a means to set the segment size in
> Why not to do so as a part of CreateTabletAndStartClusterWithSchema() ?
Added a comment explaining.
--
To view, visit http://gerrit.cloudera.org:8080/17787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iabd9fe33f9fa21f62b513d7607bf62b6fce6bb59
Gerrit-Change-Number: 17787
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 18 Aug 2021 20:48:24 +0000
Gerrit-HasComments: Yes