You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/06/15 17:32:07 UTC
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Hello David Ribeiro Alves, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7192
to review the following change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 195 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/1
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#9).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 211 insertions(+), 258 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/9
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
PS8, Line 21: #include <gflags/gflags.h>
nit: could you move it after std includes, please?
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS8, Line 21: #include <boost/optional.hpp>
: #include <gflags/gflags.h>
> nit: while you at it, could you please move these after the std includes?
Done
PS8, Line 23: #include <iostream>
> nit: it seems only ostream is needed here:
Done
PS8, Line 154: using std::shared_ptr;
: using std::unique_ptr;
> nit: not sure these are needed here
Removed
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS8, Line 20: #include <boost/optional/optional_fwd.hpp>
> nit: while you at it, could you please move this after the std includes and
Done
Line 21: #include <memory>
> nit: it would be nice to add
Done
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#7).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 200 insertions(+), 251 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/7
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#6).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 202 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/6
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS8, Line 21: #include <boost/optional.hpp>
: #include <gflags/gflags.h>
nit: while you at it, could you please move these after the std includes?
PS8, Line 23: #include <iostream>
nit: it seems only ostream is needed here:
#include <ostream>
PS8, Line 154: using std::shared_ptr;
: using std::unique_ptr;
nit: not sure these are needed here
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS8, Line 20: #include <boost/optional/optional_fwd.hpp>
nit: while you at it, could you please move this after the std includes and before Kudu-specific ones?
Line 21: #include <memory>
nit: it would be nice to add
#include <iosfwd>
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#4).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 198 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/4
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
PS8, Line 21: #include <gflags/gflags.h>
> nit: could you move it after std includes, please?
Done
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:
PS8, Line 161: for (int i = 0; i < config_.peers_size(); i++) {
: scoped_refptr<ConsensusMetadata> cmeta;
: RETURN_NOT_OK(cmeta_managers_[i]
Does it make sense to add
CHECK_EQ(config_.peers_size(), cmeta_managers_.size());
CHECK_EQ(config_.peers_size(), fs_managers_.size());
prior to the cycle?
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Reviewed-on: http://gerrit.cloudera.org:8080/7192
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 211 insertions(+), 258 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7192/2//COMMIT_MSG
Commit Message:
PS2, Line 12: simplies
typo
http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS2, Line 226: failure_monitor_.reset(new RandomizedFailureMonitor(GetRandomSeed32(),
: GetFailureMonitorCheckMeanMs(),
: GetFailureMonitorCheckStddevMs()));
: failure_detector_.reset(new TimedFailureDetector(MonoDelta::FromMilliseconds(
: FLAGS_raft_heartbeat_interval_ms *
: FLAGS_leader_failure_max_missed_heartbeat_periods)));
these can be moved to the ctor, right?
PS2, Line 238: LockGuard l(lock_);
: current_term = GetCurrentTermUnlocked();
why is the lock needed here? just because we have an assert?
http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS2, Line 788:
this is no longer needed? maybe spell it out in the commit message?
PS2, Line 125: Status Init();
should probably say something about threading in Init() and Start() as they are lockless
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7192/8/src/kudu/consensus/raft_consensus_quorum-test.cc
File src/kudu/consensus/raft_consensus_quorum-test.cc:
PS8, Line 161: for (int i = 0; i < config_.peers_size(); i++) {
: scoped_refptr<ConsensusMetadata> cmeta;
: RETURN_NOT_OK(cmeta_managers_[i]
> Does it make sense to add
Done
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#2).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 194 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/2
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7192/9/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
Line 220: queue_.reset(new PeerMessageQueue(std::move(metric_entity),
> warning: passing result of std::move() as a const reference argument; no mo
I'm going to ignore this (benign) warning for now; I'll deal with it in a follow up patch.
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#3).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 198 insertions(+), 245 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/3
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7192
to look at the new patch set (#5).
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
consensus: Get rid of RaftConsensus::Create()
It is no longer useful to have a factory method for RaftConsensus,
particularly since we need to be able to separate creation from
initialization in a follow-up patch. This is a minor refactor that
removes Create() and simplifies a couple of things.
Also adds an additional lifecycle state to RaftConsensus (kNew) for
validation purposes.
Also remove unused MemTracker instance from RaftConsensus.
Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/tablet/tablet_replica.cc
4 files changed, 207 insertions(+), 249 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7192/5
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of RaftConsensus::Create()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Get rid of RaftConsensus::Create()
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7192/2//COMMIT_MSG
Commit Message:
PS2, Line 12: simplies
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
PS2, Line 226: failure_monitor_.reset(new RandomizedFailureMonitor(GetRandomSeed32(),
: GetFailureMonitorCheckMeanMs(),
: GetFailureMonitorCheckStddevMs()));
: failure_detector_.reset(new TimedFailureDetector(MonoDelta::FromMilliseconds(
: FLAGS_raft_heartbeat_interval_ms *
: FLAGS_leader_failure_max_missed_heartbeat_periods)));
> these can be moved to the ctor, right?
We don't want these running for tombstoned tablets, so no.
PS2, Line 238: LockGuard l(lock_);
: current_term = GetCurrentTermUnlocked();
> why is the lock needed here? just because we have an assert?
Yeah, but there's not much harm. It's not worth changing because Start() will have to be thread safe once we have tombstoned voting.
http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:
PS2, Line 788:
> this is no longer needed? maybe spell it out in the commit message?
It was never used. I'll add a note.
PS2, Line 125: Status Init();
> should probably say something about threading in Init() and Start() as they
Done
--
To view, visit http://gerrit.cloudera.org:8080/7192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes