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 2020/06/02 23:47:22 UTC
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/16027
to review the following change.
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 296 insertions(+), 156 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16027/1
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16027
to look at the new patch set (#5).
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 290 insertions(+), 168 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16027/5
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16027
to look at the new patch set (#3).
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 281 insertions(+), 163 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16027/3
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16027 )
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
Patch Set 4: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.h
File src/kudu/tablet/tablet_replica-test-base.h:
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.h@63
PS4, Line 63: const scoped_refptr<TabletReplica>& tablet_replica() { return tablet_replica_; }
nit: add 'const' specifier?
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc
File src/kudu/tablet/tablet_replica-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@83
PS4, Line 83: tablet_replica_->Shutdown();
Could tablet_replica_ be nullptr here (e.g., SetUp() bailed out before reaching SetUpReplica())? If so, maybe move call to Shutdown() under
if (tablet_replica_)
scope?
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@85
PS4, Line 85: apply_pool_->Shutdown();
What about raft_pool_? If it doesn't need to be shutdown here, maybe it's worth adding a comment to explain the reason?
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@89
PS4, Line 89: void
nit: maybe, make this method return Status?
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 03 Jun 2020 05:01:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16027 )
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.h
File src/kudu/tablet/tablet_replica-test-base.h:
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.h@63
PS4, Line 63: const scoped_refptr<TabletReplica>& tablet_replica() const { return tablet_replica_; }
> nit: add 'const' specifier?
Done
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc
File src/kudu/tablet/tablet_replica-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@83
PS4, Line 83: ASSERT_OK(SetUpReplica());
> Could tablet_replica_ be nullptr here (e.g., SetUp() bailed out before reac
I don't think that's possible. If SetUp() fails, the test exits regardless.
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@85
PS4, Line 85:
> What about raft_pool_? If it doesn't need to be shutdown here, maybe it's
Done
http://gerrit.cloudera.org:8080/#/c/16027/4/src/kudu/tablet/tablet_replica-test-base.cc@89
PS4, Line 89: ap
> nit: maybe, make this method return Status?
Done
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 03 Jun 2020 19:32:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16027
to look at the new patch set (#2).
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 281 insertions(+), 162 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16027/2
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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-Reviewer: Tidy Bot (241)
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16027 )
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 03 Jun 2020 22:02:13 +0000
Gerrit-HasComments: No
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16027
to look at the new patch set (#4).
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 281 insertions(+), 163 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16027/4
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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-Reviewer: Tidy Bot (241)
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16027 )
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
tablet_replica-test: refactor to pull out some useful code
In preparation for testing a new system table that is backed by tablet
replicas, this pulls out some of the tablet replica initialization code
used in TabletReplicaTest to facilitate its reuse.
There are no major functional changes in this patch.
Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Reviewed-on: http://gerrit.cloudera.org:8080/16027
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.h
A src/kudu/tablet/tablet_replica-test-base.cc
A src/kudu/tablet/tablet_replica-test-base.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.h
6 files changed, 290 insertions(+), 168 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
Gerrit-PatchSet: 6
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-Reviewer: Tidy Bot (241)
[kudu-CR] tablet replica-test: refactor to pull out some useful code
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16027 )
Change subject: tablet_replica-test: refactor to pull out some useful code
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.h
File src/kudu/tablet/tablet_replica-test-base.h:
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.h@45
PS1, Line 45: explicit TabletReplicaTestBase(const Schema& schema)
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.cc
File src/kudu/tablet/tablet_replica-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.cc@56
PS1, Line 56: using kudu::rpc::MessengerBuilder;
> warning: using decl 'Messenger' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.cc@60
PS1, Line 60: using std::shared_ptr;
> warning: using decl 'TabletHarness' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16027/1/src/kudu/tablet/tablet_replica-test-base.cc@64
PS1, Line 64:
> warning: using decl 'unique_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/16027/2/src/kudu/tablet/tablet_replica-test.cc
File src/kudu/tablet/tablet_replica-test.cc:
http://gerrit.cloudera.org:8080/#/c/16027/2/src/kudu/tablet/tablet_replica-test.cc@84
PS2, Line 84: using kudu::log::LogOptions;
> warning: using decl 'LogOptions' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/16027
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I912221d73b4d83bb967d91ca7592618b4a89d74c
Gerrit-Change-Number: 16027
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-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 02 Jun 2020 23:56:09 +0000
Gerrit-HasComments: Yes