You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/12/19 23:19:29 UTC
[kudu-CR] [consensus] test-only option for replica replacement
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8889
Change subject: [consensus] test-only option for replica replacement
......................................................................
[consensus] test-only option for replica replacement
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/master/catalog_manager.cc
6 files changed, 606 insertions(+), 354 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8889/1
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
[consensus] add unsafe gflag to bypass "safe to evict" logic
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Reviewed-on: http://gerrit.cloudera.org:8080/8889
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
7 files changed, 504 insertions(+), 373 deletions(-)
Approvals:
Alexey Serbin: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8889
to look at the new patch set (#4).
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
[consensus] add unsafe gflag to bypass "safe to evict" logic
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
7 files changed, 503 insertions(+), 381 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8889/4
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8889/2/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/2/src/kudu/consensus/quorum_util-test.cc@957
PS2, Line 957: const auto kPolicies = { MHP_H, MHP_I }
> so this is still repeated a bunch of times. can we put it at the top of the
I decided to make it a parameterized test instead.
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 05 Jan 2018 00:53:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8889/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8889/3//COMMIT_MSG@7
PS3, Line 7: [consensus] test-only option for replica replacement
> nit: how about titling this something a little more specific for the git lo
Done
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util-test.cc@a621
PS3, Line 621:
> it seems like a lot of code moved around in this patch. is that preventable
I did some re-ordering to keep parameterized stuff together: I didn't realize that would be a pain to review. Sorry about that -- I re-ordered the changes so they are easier to review now.
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util.h
File src/kudu/consensus/quorum_util.h:
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util.h@110
PS3, Line 110: //MajorityHealthPolicy policy = MajorityHealthPolicy::HONOR);
> remove?
Done
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 06 Jan 2018 00:52:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc@a454
PS4, Line 454:
:
:
:
:
:
:
:
:
> Was this test removed?
Woops, it's good catch -- I remember removing some duplicate scenario, but apparently it's not a duplicate (the value of the PROMOTE attribute is different from the case above).
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc@834
PS4, Line 834: :
> nit: missing space before colon
Done
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util.h
File src/kudu/consensus/quorum_util.h:
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util.h@40
PS4, Line 40: // (this applies to both the current and the result configuration).
> I don't think this is true, it should only apply to the resulting configura
Yep, it seems that was a brainfart. Sure, that's true only for the resulting config.
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 08 Jan 2018 19:42:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8889
to look at the new patch set (#5).
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
[consensus] add unsafe gflag to bypass "safe to evict" logic
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
7 files changed, 504 insertions(+), 373 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8889/5
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 1: Verified+1
The long-standing flakiness:
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/integration-tests/raft_consensus-itest.cc:1625: Failure
Failed
Bad status: Illegal state: Leader has not yet committed an operation in its own term
I addressed the issue in PS5 of the following patch: http://gerrit.cloudera.org:8080/8856
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Wed, 20 Dec 2017 22:45:45 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8889
to look at the new patch set (#3).
Change subject: [consensus] test-only option for replica replacement
......................................................................
[consensus] test-only option for replica replacement
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
7 files changed, 879 insertions(+), 753 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8889/3
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Patch Set 5: Verified+1
Unrelated flake in MultiThreadedHybridClockTabletTest/3.UpdateNoMergeCompaction
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 08 Jan 2018 20:26:10 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8889/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8889/3//COMMIT_MSG@7
PS3, Line 7: [consensus] test-only option for replica replacement
nit: how about titling this something a little more specific for the git log --online output, such as:
[consensus] add unsafe gflag to bypass "safe to evict" logic
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util-test.cc@a621
PS3, Line 621:
it seems like a lot of code moved around in this patch. is that preventable? it makes the code review a lot more work.
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util.h
File src/kudu/consensus/quorum_util.h:
http://gerrit.cloudera.org:8080/#/c/8889/3/src/kudu/consensus/quorum_util.h@110
PS3, Line 110: //MajorityHealthPolicy policy = MajorityHealthPolicy::HONOR);
remove?
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 06 Jan 2018 00:23:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util-test.cc@958
PS1, Line 958:
> nit: since this is repeated many times, consider extracting {MHP_H, MHP_I}
Done
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@448
PS1, Line 448: is_under_replicated &&
: (num_voters_healthy >= MajoritySize(num_voters_total) ||
: policy == MajorityHealthPolicy::IGNORE);
> how about:
Done
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@640
PS1, Line 640: need_to_evict_non_voter &&
: (num_voters_healthy >= MajoritySize(num_voters_total) ||
: policy == MajorityHealthPolicy::IGNORE);
> how about:
Done
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@683
PS1, Line 683: num_voters_total > replication_factor &&
: (num_voters_healthy >= MajoritySize(num_voters_total - 1) ||
: policy == MajorityHealthPolicy::IGNORE);
> nit: how about
Done
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/raft_consensus.cc@134
PS1, Line 134: desired Raf
> s/appropriate/desired/
Done
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 23 Dec 2017 01:17:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8889/2/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/2/src/kudu/consensus/quorum_util-test.cc@957
PS2, Line 957: const auto kPolicies = { MHP_H, MHP_I }
so this is still repeated a bunch of times. can we put it at the top of the file maybe around line 50?
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 23 Dec 2017 18:31:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8889
to look at the new patch set (#2).
Change subject: [consensus] test-only option for replica replacement
......................................................................
[consensus] test-only option for replica replacement
Replica replacement: added an option to attempt Raft configuration
update even in absence of healthy majority of tablet replicas.
This option is for testing purposes: as we could see during latest load
testing, an attempt to change the configuration under such unusual
circumstances allowed for exposing some bugs.
In run-time, the option is controlled by the newly introduced flag
--raft_attempt_to_replace_replica_without_majority. It affects both
3-4-3 and 3-2-3 replica replacement schemes.
Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/master/catalog_manager.cc
7 files changed, 628 insertions(+), 366 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/8889/2
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc@a454
PS4, Line 454:
:
:
:
:
:
:
:
:
Was this test removed?
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util-test.cc@834
PS4, Line 834: :
nit: missing space before colon
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util.h
File src/kudu/consensus/quorum_util.h:
http://gerrit.cloudera.org:8080/#/c/8889/4/src/kudu/consensus/quorum_util.h@40
PS4, Line 40: // (this applies to both the current and the result configuration).
I don't think this is true, it should only apply to the resulting configuration.
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 06 Jan 2018 01:36:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] test-only option for replica replacement
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] test-only option for replica replacement
......................................................................
Patch Set 1: Code-Review+1
(5 comments)
looks good, just nits
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util-test.cc@958
PS1, Line 958: for (auto policy : {MHP_H, MHP_I})
nit: since this is repeated many times, consider extracting {MHP_H, MHP_I} into a const at the top of the file and use that instead:
constexpr auto kPolicies = { MHP_H, MHP_I };
for (auto policy : kPolicies) {
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@448
PS1, Line 448: is_under_replicated &&
: (policy == MajorityHealthPolicy::HONOR
: ? num_voters_healthy >= MajoritySize(num_voters_total) : true);
how about:
is_under_replicated &&
(num_voters_healthy >= MajoritySize(num_voters_total) || policy == MajorityHealthPolicy::IGNORE);
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@640
PS1, Line 640: need_to_evict_non_voter &&
: (policy == MajorityHealthPolicy::HONOR
: ? num_voters_healthy >= MajoritySize(num_voters_total) : true);
how about:
need_to_evict_non_voter &&
(num_voters_healthy >= MajoritySize(num_voters_total) || policy == MajorityHealthPolicy::IGNORE);
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/quorum_util.cc@683
PS1, Line 683: num_voters_total > replication_factor &&
: (policy == MajorityHealthPolicy::HONOR
: ? num_voters_healthy >= MajoritySize(num_voters_total - 1) : true);
nit: how about
num_voters_total > replication_factor &&
(num_voters_healthy >= MajoritySize(num_voters_total - 1) || policy == MajorityHealthPolicy::IGNORE);
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:
http://gerrit.cloudera.org:8080/#/c/8889/1/src/kudu/consensus/raft_consensus.cc@134
PS1, Line 134: appropriate
s/appropriate/desired/
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 22 Dec 2017 21:18:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus] add unsafe gflag to bypass "safe to evict" logic
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8889 )
Change subject: [consensus] add unsafe gflag to bypass "safe to evict" logic
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8889
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f89b6c584296e3da5047475c5c86c4cb1118ad0
Gerrit-Change-Number: 8889
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jan 2018 01:21:43 +0000
Gerrit-HasComments: No