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/11/29 10:19:08 UTC
[kudu-CR] WIP [itest] one more test for 3-4-3 re-replication scheme
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8679
Change subject: WIP [itest] one more test for 3-4-3 re-replication scheme
......................................................................
WIP [itest] one more test for 3-4-3 re-replication scheme
This is a new test (disabled for a while) to make sure the system is
able to recover from the situation when a first attempt to replica
a failed replica fails at every tablet server.
WIP: make it work
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
2 files changed, 104 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/1
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#5).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tools/kudu-admin-test.cc
4 files changed, 709 insertions(+), 91 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/5
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
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>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [quorum util] update criteria for replica eviction
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/8679
to look at the new patch set (#2).
Change subject: [quorum_util] update criteria for replica eviction
......................................................................
[quorum_util] update criteria for replica eviction
Updated the replica eviction criteria in consensus::CanEvictReplica()
to address a scenario where newly added replicas fail one after
another before the original failed replica evicted. This is to make
sure it does not end up with placing failed replicas at every available
tablet server, deadlocking the replacement process.
Added corresponding unit tests to cover the updated behavior of
consensus::CanEvictReplica().
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
3 files changed, 298 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/2
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 8:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG@23
PS8, Line 23: since now it should not be flaky anymore.
> I tried this and it failed 3/200 on dist-test with 4 stress threads: http:/
Interesting. Maybe, the new version (PS8) is better, but I got better results with it:
debug:
http://dist-test.cloudera.org//job?job_id=aserbin.1513034972.132262
release:
http://dist-test.cloudera.org//job?job_id=aserbin.1513035867.144075
The only failure was due to some unrelated problem (probably, due to the lack of fsync during tests):
Bad status: Corruption: Mismatch found for index 213, [T 42cee95c5ff24a05a25cbbfb09eba052=3, T da86f3a13527438095bf1ade670a8f65=-1, T 9b08f8d5c1794659bf53f74ec08af43d=3, T 6bfbb7f0aa9b4eafb51cd202f0ac2b60=-1, T 3ea1b32f1a4e42a8afe7c4174346444b=2]
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util-test.cc@991
PS8, Line 991: {
> what do you think about adding this test?
As per our off-line discussion, I think it's a good idea.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@497
PS8, Line 497: !has_replace) {
: ++num_voters_viable;
> nit: put this inside the if-statement after line 489?
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@600
PS8, Line 600: num_voters_viable >= replication_factor
> This is a bit of a nitpick, but I think this should be:
The idea was to have a common filter applied later at line 612 -- there might be more conditions, etc. All right, if it seems confusing, then I'll add that here.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@610
PS8, Line 610: commence
> nit: commit
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@612
PS8, Line 612: num_non_voters_total > 0 &&
> see my comment on line 600
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@630
PS8, Line 630: !voter_unknown_health.empty();
> nit: consider indenting this line to line up with "num_voters_viable >= rep
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@635
PS8, Line 635: need_to_evict_voter |=
: (num_voters_viable >= replication_factor ||
: (num_voters_healthy > replication_factor &&
: num_voters_healthy > num_voters_with_replace)) &&
: !(num_voters_with_replace == 1 && leader_with_replace);
> I found some of this a little confusing as to why they were needed, particu
I think it's a good idea. I also updated the overall logic for the eviction of replicas having the 'replace' attribute set. I think it's clearer now.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:
PS8:
I think this is just a part of some other patch in progress. I'll add these changes as a part of some other patch.
Thank you for reviewing this -- I'll address your comments on this file in another patch.
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 8
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 00:11:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [quorum util] update criteria for non-voter replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#3).
Change subject: [quorum_util] update criteria for non-voter replica eviction
......................................................................
[quorum_util] update criteria for non-voter replica eviction
Updated the non-voter replica eviction criteria to address a scenario
where newly added non-voter replicas fail one after another before the
original failed voter replica is evicted. This is to make sure the
replacement process does not end up with placing failed non-voter
replicas at every available tablet server, deadlocking the replacement
process.
Added corresponding unit tests to cover the updated behavior of the
consensus::CanEvictReplica() utility function.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
3 files changed, 272 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/3
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [quorum util] update criteria for replica eviction
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: [quorum_util] update criteria for replica eviction
......................................................................
Patch Set 2:
(17 comments)
I think we should consider splitting this into two patches. The non-voter changes as outlined in the commit message are important and I think non-controversial.
I am not sure about the voter eviction changes because a HEALTHY health report does not currently guarantee that a replica is not lagging, at least a little bit. I think we need to be more conservative about evicting voters right now and only evict a voter if we have # replication factory HEALTHY voters.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@41
PS2, Line 41: constexpr auto U = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
> warning: invalid case style for global constant 'U' [readability-identifier
how about // NOLINT here and below?
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@45
PS2, Line 45: const auto health_statuses = { '?', '-', '+' };
> warning: invalid case style for global constant 'health_statuses' [readabil
kHeatlhStatuses?
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@734
PS2, Line 734: EXPECT_EQ("C", to_evict)
how about: EXPECT(to_evict == "C" || to_evict == "D") << to_evict; ?
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@745
PS2, Line 745: 3
magic number, can you pull this out into a constant? Such as const auto kReplicationFactor = 3;
The other tests are so short that it's less of a concern but "3" appears a couple dozen times in this scenario so the logic would be easier to follow with replication factor as a constant.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@773
PS2, Line 773: prior B is evicted
prior to B getting evicted
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@774
PS2, Line 774: the failed voter
who?
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@779
PS2, Line 779: ASSERT_EQ("B", to_evict);
Why is "B" the correct choice instead of "D" here? aren't they equally good choices and this is implementation specific? Wouldn't this test become brittle if we changed the rules later?
Perhaps ASSERT(to_evict == "B" || to_evict == "D") << to_evict; ?
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@531
PS2, Line 531: which
that
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@532
PS2, Line 532: :
, while
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@533
PS2, Line 533: which
whose
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@536
PS2, Line 536: in 'bad'
with FAILED
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@537
PS2, Line 537: REPLACE
'replace'
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@539
PS2, Line 539: .
, while replacing failed non-voters with healthy non-voters as aggressively as possible.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@540
PS2, Line 540: when attempting an eviction, it's better to be certain about the ability
: // to carry out the operation.
we want to be sure that an eviction can succeed before initiating it.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@545
PS2, Line 545: greater
greater than
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@547
PS2, Line 547: bad
failed
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@557
PS2, Line 557: (num_voters_healthy >= replication_factor && num_voters_total > replication_factor) ||
: (num_voters_total > replication_factor && !voter_failed.empty() &&
: num_voters_healthy >= MajoritySize(num_voters_total - 1));
nit: consider hoisting out one of the conditions, just for readability:
num_voters_total > replication_factor &&
(num_voters_healthy >= replication_factor ||
(!voter_failed.empty() && num_voters_healthy >= MajoritySize(num_voters_total - 1)))
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 05 Dec 2017 02:21:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 11: Verified+1
unrelated flake in TabletServerDiskFailureTest.TestRandomOpSequence
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 05:31:46 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 8:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG@23
PS8, Line 23: since now it should not be flaky anymore.
I tried this and it failed 3/200 on dist-test with 4 stress threads: http://dist-test.cloudera.org/job?job_id=mpercy.1512786373.82470 (the other 5 failures are different tests)
That said, a 1.5% failure rate shouldn't kill this patch, we could do it in a follow-up.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util-test.cc@991
PS8, Line 991: {
what do you think about adding this test?
+ for (auto replica_health : kHealthStatuses) {
+ SCOPED_TRACE(Substitute("replica health status '$0'", replica_health));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "C", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "D", V, replica_health, {{"REPLACE", true}});
+ string to_evict;
+ EXPECT_TRUE(CanEvictReplica(config, "A", 3, &to_evict));
+ if (replica_health == '+') {
+ EXPECT_NE("A", to_evict);
+ } else {
+ EXPECT_EQ("D", to_evict);
+ }
+ EXPECT_TRUE(IsUnderReplicated(config, 3));
+ }
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@497
PS8, Line 497: !has_replace) {
: ++num_voters_viable;
nit: put this inside the if-statement after line 489?
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@600
PS8, Line 600: num_voters_viable >= replication_factor
This is a bit of a nitpick, but I think this should be:
num_voters_viable >= replication_factor && num_non_voters_total > 0
I found it confusing doing the latter check on line 612 since it's redundant with line 606.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@610
PS8, Line 610: commence
nit: commit
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@612
PS8, Line 612: num_non_voters_total > 0 &&
see my comment on line 600
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@630
PS8, Line 630: !voter_unknown_health.empty();
nit: consider indenting this line to line up with "num_voters_viable >= replication_factor &&" on the previous line
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/consensus/quorum_util.cc@635
PS8, Line 635: need_to_evict_voter |=
: (num_voters_viable >= replication_factor ||
: (num_voters_healthy > replication_factor &&
: num_voters_healthy > num_voters_with_replace)) &&
: !(num_voters_with_replace == 1 && leader_with_replace);
I found some of this a little confusing as to why they were needed, particularly why we should consider num_voters_viable == replication_factor as a trigger for eviction. What do you think about this small modification:
need_to_evict_voter |=
!(num_voters_with_replace == 1 && leader_with_replace) &&
(num_voters_with_replace > replication_factor ||
(num_voters_with_replace > 0 &&
num_voters_healthy > replication_factor));
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1449
PS8, Line 1449: string follower_uuid;
Please add comment:
// Cause follower 'follower_uuid' to fail.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1498
PS8, Line 1498: fallen
nit: that fell
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1521
PS8, Line 1521: ASSERT_OK(WaitForReplicasReportedToMaster(cluster_->master_proxy(),
: kReplicasNum,
: tablet_id_,
: kTimeout,
: WAIT_FOR_LEADER,
: ANY_REPLICA,
: &has_leader,
: &tablet_locations));
why does this retrying method need to be inside an ASSERT_EVENTUALLY?
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1530
PS8, Line 1530: ASSERT_EVENTUALLY([&] {
are the nested ASSERT_EVENTUALLY blocks intentional?
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1537
PS8, Line 1537: // The original voter replica fallen behind the WAL catchup threshold
: // should be evicted.
: ASSERT_FALSE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "fallen behind WAL replica UUID: " << follower_uuid;
: // The first non-voter replica failed during the tablet copy phase
: // should not be present.
: ASSERT_FALSE(IsRaftConfigMember(ts0->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "failed tablet copy replica UUID: " << ts0->uuid();
: // The tablet copy on the restarted server should succeed and this replica
: // should replace the original failed replica.
: ASSERT_TRUE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "new replacement replica UUID: " << ts1->uuid();
can't this last bit be inside the inner ASSERT_EVENTUALLY?
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 8
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 09 Dec 2017 04:14:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc@622
PS10, Line 622: num_non_voters_total > 0 &&
> can't we remove this now that it was moved to line 610?
Woops, that's slipped through cracks.
Done.
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc@654
PS10, Line 654: It's a special case
> nit: In the special case
Done
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 10
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 04:59:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [quorum util] update criteria for non-voter replica eviction
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: [quorum_util] update criteria for non-voter replica eviction
......................................................................
Patch Set 3:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@64
PS3, Line 64: FAIL
LOG(FATAL) so we don't have to put NO_FATALS() around the SetOverallHealth() calls
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@100
PS3, Line 100: auto* peers = config->mutable_peers();
: bool found_peer = false;
: for (auto& peer : *peers) {
: if (peer.permanent_uuid() == peer_uuid) {
: found_peer = true;
: peer.set_member_type(V);
: //peer.mutable_attrs()->clear_promote();
: peer.mutable_attrs()->set_promote(false);
: break;
: }
: }
: if (!found_peer) {
: FAIL() << peer_uuid << ": peer is not in the config";
: }
we can write this in fewer lines using GetRaftConfigMember():
RaftPeerPB* peer;
ASSERT_OK(GetRaftConfigMember(config->mutable_peers(), peer_uuid, peer));
peer->set_member_type(V);
peer->mutable_attrs()->set_promote(false);
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@116
PS3, Line 116: static void RemovePeer(RaftConfigPB* config, const string& peer_uuid) {
we can just use bool RemoveFromRaftConfig(RaftConfigPB* config, const string& uuid);
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@135
PS3, Line 135: if (!s.ok()) {
: FAIL()
why not just ASSERT_OK() ?
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@530
PS3, Line 530: for (char replace_health : kHealthStatuses) {
this test has nothing to do with non-voters, right?
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@609
PS3, Line 609: .
if we have enough healthy voters to commit the config change.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@661
PS3, Line 661: ASSERT_TRUE
This one confuses me. Shouldn't this be ASSERT_FALSE because we don't have enough healthy voters to commit the config change? we only have 1/2 healthy voters here, while MajoritySize(2) == 2.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@727
PS3, Line 727: SetPeerHealth
nit: NO_FATALS() here and below
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@731
PS3, Line 731: Adding a non-voter for replacement
nit: This comment could be a little clearer written as: Add a non-voter to replace B.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@742
PS3, Line 742: PromotePeer
nit: NO_FATALS() here and below
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@753
PS3, Line 753: SetPeerHealth(&config, "D", '-');
should we also test the (D, '?') case before it goes fully FAILED?
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@754
PS3, Line 754: ASSERT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
nit: add comment: // We cannot evict because we don't have enough healthy voters to commit an eviction config change.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@757
PS3, Line 757: EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
: EXPECT_TRUE(IsUnderReplicated(config, kReplicationFactor));
this duplicates the assertions above
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@775
PS3, Line 775: // The processs converges: 3 voter replicas, all are healthy.
nit: it's a little early to say this in this comment. This state would occur after this next operation at the end of the test.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@779
PS3, Line 779: EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
perhaps also assert that we cannot evict any nodes here, either.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc@531
PS3, Line 531: not to evict
to not evict
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc@544
PS3, Line 544: // * A voter replica may be evicted only if the number of voter replicas in
: // good health without the REPLACE attribute is greater or equal to the
: // specified replication factor.
is this comment relevant to this patch?
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 23:44:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#7).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tools/kudu-admin-test.cc
4 files changed, 758 insertions(+), 91 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/7
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#10).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
As an additional touch, updated the FailedTabletCopy scenario of the
RaftConsensusNonVoterITest to provide more comprehensive coverage.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-admin-test.cc
5 files changed, 848 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/10
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 10
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-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:
PS8:
> I think this is just a part of some other patch in progress. I'll add thes
Actually, as I found after some digging into my git operations, this should be a part of this patch. I addressed your comments in PS10 of this patch.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1449
PS8, Line 1449: string follower_uuid;
> Please add comment:
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1498
PS8, Line 1498: fallen
> nit: that fell
Done
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1521
PS8, Line 1521: ASSERT_OK(WaitForReplicasReportedToMaster(cluster_->master_proxy(),
: kReplicasNum,
: tablet_id_,
: kTimeout,
: WAIT_FOR_LEADER,
: ANY_REPLICA,
: &has_leader,
: &tablet_locations));
> why does this retrying method need to be inside an ASSERT_EVENTUALLY?
This method does not retry if master replies with something like 'catalog manager is not running'. I'm not sure whether it's safe to change the implementation to retry in such case, though.
But overall -- the whole sequence needs to be under ASSERT_EVENTUALLY since it's hard to predict when the replacement process starts and ends.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1530
PS8, Line 1530: ASSERT_EVENTUALLY([&] {
> are the nested ASSERT_EVENTUALLY blocks intentional?
Good point -- having the outer ASSERT_EVENTUALLY it's possible to drop the embedded one.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1537
PS8, Line 1537: // The original voter replica fallen behind the WAL catchup threshold
: // should be evicted.
: ASSERT_FALSE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "fallen behind WAL replica UUID: " << follower_uuid;
: // The first non-voter replica failed during the tablet copy phase
: // should not be present.
: ASSERT_FALSE(IsRaftConfigMember(ts0->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "failed tablet copy replica UUID: " << ts0->uuid();
: // The tablet copy on the restarted server should succeed and this replica
: // should replace the original failed replica.
: ASSERT_TRUE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "new replacement replica UUID: " << ts1->uuid();
> can't this last bit be inside the inner ASSERT_EVENTUALLY?
I think it's enough to have just one outer ASSERT_EVENTUALLY.
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 8
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 02:30:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 6: Verified+1
unrelated flake in tablet_copy-itest
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 06:44:25 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#8).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-admin-test.cc
5 files changed, 765 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/8
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 8
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-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
As an additional touch, updated the FailedTabletCopy scenario of the
RaftConsensusNonVoterITest to provide more comprehensive coverage.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Reviewed-on: http://gerrit.cloudera.org:8080/8679
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-admin-test.cc
5 files changed, 847 insertions(+), 96 deletions(-)
Approvals:
Alexey Serbin: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8679/8//COMMIT_MSG@23
PS8, Line 23: since now it should not be flaky anymore.
> Interesting. Maybe, the new version (PS8) is better, but I got better resu
I meant PS9.
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 8
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 00:29:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#9).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-admin-test.cc
4 files changed, 794 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/9
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 9
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-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#11).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
As an additional touch, updated the FailedTabletCopy scenario of the
RaftConsensusNonVoterITest to provide more comprehensive coverage.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tools/kudu-admin-test.cc
5 files changed, 847 insertions(+), 96 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/11
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 11
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-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 18:36:00 +0000
Gerrit-HasComments: No
[kudu-CR] [quorum util] update criteria for non-voter replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#4).
Change subject: [quorum_util] update criteria for non-voter replica eviction
......................................................................
[quorum_util] update criteria for non-voter replica eviction
Updated the non-voter replica eviction criteria to address a scenario
where newly added non-voter replicas fail one after another before the
original failed voter replica is evicted. This is to make sure the
replacement process does not end up with placing failed non-voter
replicas at every available tablet server, deadlocking the replacement
process.
Added corresponding unit tests to cover the updated behavior of the
consensus::CanEvictReplica() utility function.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
3 files changed, 263 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/4
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
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-Reviewer: Tidy Bot
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
Patch Set 10: Code-Review+1
(4 comments)
just a couple last nits, this is basically ready to go
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc@622
PS10, Line 622: num_non_voters_total > 0 &&
can't we remove this now that it was moved to line 610?
http://gerrit.cloudera.org:8080/#/c/8679/10/src/kudu/consensus/quorum_util.cc@654
PS10, Line 654: It's a special case
nit: In the special case
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1521
PS8, Line 1521: bool has_leader = false;
: TabletLocationsPB tablet_locations;
: ASSERT_OK(WaitForReplicasReportedToMaster(cluster_->master_proxy(),
: kReplicasNum,
: tablet_id_,
: kTimeout,
: WAIT_FOR_LEADER,
: ANY_REPLICA,
> This method does not retry if master replies with something like 'catalog m
I don't think that was intentional but Rev 10 seems fine to me.
http://gerrit.cloudera.org:8080/#/c/8679/8/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1537
PS8, Line 1537: ASSERT_FALSE(IsRaftConfigMember(follower_uuid, cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "fell behind WAL replica UUID: " << follower_uuid;
: // The first non-voter replica failed during the tablet copy phase
: // should not be present.
: ASSERT_FALSE(IsRaftConfigMember(ts0->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "failed tablet copy replica UUID: " << ts0->uuid();
: // The tablet copy on the restarted server should succeed and this replica
: // should replace the original failed replica.
: ASSERT_TRUE(IsRaftConfigMember(ts1->uuid(), cstate.committed_config()))
: << pb_util::SecureDebugString(cstate.committed_config())
: << "new replacement replica UUID: " << ts1->uuid();
: });
: }
> I think it's enough to have just one outer ASSERT_EVENTUALLY.
+1
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 10
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Dec 2017 04:36:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [quorum util] update criteria for non-voter replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: [quorum_util] update criteria for non-voter replica eviction
......................................................................
Patch Set 2:
(20 comments)
> (17 comments)
>
> I think we should consider splitting this into two patches. The
> non-voter changes as outlined in the commit message are important
> and I think non-controversial.
>
> I am not sure about the voter eviction changes because a HEALTHY
> health report does not currently guarantee that a replica is not
> lagging, at least a little bit. I think we need to be more
> conservative about evicting voters right now and only evict a voter
> if we have # replication factory HEALTHY voters.
Done.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@41
PS2, Line 41: constexpr auto U = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
> warning: invalid case style for global constant 'U' [readability-identifier
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@41
PS2, Line 41: constexpr auto U = RaftPeerPB::UNKNOWN_MEMBER_TYPE;
> how about // NOLINT here and below?
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@42
PS2, Line 42: constexpr auto V = RaftPeerPB::VOTER;
> warning: invalid case style for global constant 'V' [readability-identifier
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@45
PS2, Line 45: const auto health_statuses = { '?', '-', '+' };
> kHeatlhStatuses?
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@45
PS2, Line 45: const auto health_statuses = { '?', '-', '+' };
> warning: invalid case style for global constant 'health_statuses' [readabil
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@734
PS2, Line 734: EXPECT_EQ("C", to_evict)
> how about: EXPECT(to_evict == "C" || to_evict == "D") << to_evict; ?
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@745
PS2, Line 745: 3
> magic number, can you pull this out into a constant? Such as const auto kRe
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@773
PS2, Line 773: prior B is evicted
> prior to B getting evicted
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@774
PS2, Line 774: the failed voter
> who?
That was about replica D.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util-test.cc@779
PS2, Line 779: ASSERT_EQ("B", to_evict);
> Why is "B" the correct choice instead of "D" here? aren't they equally good
You are right -- that's just an implementation detail. Both replicas are the candidates for eviction at this point.
Yep, that way of asserting is better. Good point.
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@531
PS2, Line 531: which
> that
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@532
PS2, Line 532: :
> , while
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@533
PS2, Line 533: which
> whose
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@536
PS2, Line 536: in 'bad'
> with FAILED
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@537
PS2, Line 537: REPLACE
> 'replace'
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@539
PS2, Line 539: .
> , while replacing failed non-voters with healthy non-voters as aggressively
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@540
PS2, Line 540: when attempting an eviction, it's better to be certain about the ability
: // to carry out the operation.
> we want to be sure that an eviction can succeed before initiating it.
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@545
PS2, Line 545: greater
> greater than
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@547
PS2, Line 547: bad
> failed
Done
http://gerrit.cloudera.org:8080/#/c/8679/2/src/kudu/consensus/quorum_util.cc@557
PS2, Line 557: (num_voters_healthy >= replication_factor && num_voters_total > replication_factor) ||
: (num_voters_total > replication_factor && !voter_failed.empty() &&
: num_voters_healthy >= MajoritySize(num_voters_total - 1));
> nit: consider hoisting out one of the conditions, just for readability:
Done
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 06 Dec 2017 07:54:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1097: more robust criteria for replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8679
to look at the new patch set (#6).
Change subject: KUDU-1097: more robust criteria for replica eviction
......................................................................
KUDU-1097: more robust criteria for replica eviction
Updated the replica eviction criteria to address scenarios where newly
added replicas fail (before or after promotion) one after another
prior to eviction of the original failed voter replica. With this
patch, the replacement process should not end up with placing failed
replicas at every available tablet server, deadlocking the replacement
process. Added corresponding unit tests to cover the updated behavior
of the consensus::CanEvictReplica() utility function.
Updated the replica eviction criteria for more robust handling of the
'replace' attribute. In this context, the case when the leader replica
itself is marked with the 'replace' attribute is now handled properly.
Added unit tests to cover appropriate cases.
Also, enabled the AdminCliTest.TestMoveTablet_KUDU_1097 test scenario
since now it should not be flaky anymore.
Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
---
M src/kudu/consensus/quorum_util-test.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/tools/kudu-admin-test.cc
4 files changed, 709 insertions(+), 91 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/8679/6
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
Gerrit-PatchSet: 6
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-Reviewer: Tidy Bot
[kudu-CR] [quorum util] update criteria for non-voter replica eviction
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8679 )
Change subject: [quorum_util] update criteria for non-voter replica eviction
......................................................................
Patch Set 3:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc
File src/kudu/consensus/quorum_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@64
PS3, Line 64: FAIL
> LOG(FATAL) so we don't have to put NO_FATALS() around the SetOverallHealth(
It's a test, so FAIL() is enough here. And no need to put NO_FATALS() all over the place neither.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@100
PS3, Line 100: auto* peers = config->mutable_peers();
: bool found_peer = false;
: for (auto& peer : *peers) {
: if (peer.permanent_uuid() == peer_uuid) {
: found_peer = true;
: peer.set_member_type(V);
: //peer.mutable_attrs()->clear_promote();
: peer.mutable_attrs()->set_promote(false);
: break;
: }
: }
: if (!found_peer) {
: FAIL() << peer_uuid << ": peer is not in the config";
: }
> we can write this in fewer lines using GetRaftConfigMember():
Good point, but I don't want to use ASSERT_OK() here: if the specified peer is not in the config, that's a programming error.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@116
PS3, Line 116: static void RemovePeer(RaftConfigPB* config, const string& peer_uuid) {
> we can just use bool RemoveFromRaftConfig(RaftConfigPB* config, const strin
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@135
PS3, Line 135: if (!s.ok()) {
: FAIL()
> why not just ASSERT_OK() ?
Because I don't think it's worth adding NO_FATALS() around every invocation of SetPeerHealth(): if the specified peer is not the part of the config, that must be programming error. Also, I don't want to use CHECK() here because I want the test to perform proper clean-up and there is no use of crashdump in that case.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@530
PS3, Line 530: for (char replace_health : kHealthStatuses) {
> this test has nothing to do with non-voters, right?
Right: as other sub-scenarios, it works with voter replicas. I just used consolidated the testcases below, removing trying to remove some duplicated code.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@609
PS3, Line 609: .
> if we have enough healthy voters to commit the config change.
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@661
PS3, Line 661: ASSERT_TRUE
> This one confuses me. Shouldn't this be ASSERT_FALSE because we don't have
This has changed once the criteria for the eviction updated. See https://gerrit.cloudera.org/#/c/8786/
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@727
PS3, Line 727: SetPeerHealth
> nit: NO_FATALS() here and below
I use FAIL() in the implementation of SetPeerHealth() specifically for the purpose of not adding NO_FATALS() all over the place. If SetPeerHealth() fails, that's a programming error.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@731
PS3, Line 731: Adding a non-voter for replacement
> nit: This comment could be a little clearer written as: Add a non-voter to
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@742
PS3, Line 742: PromotePeer
> nit: NO_FATALS() here and below
The same story as for SetPeerHealth().
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@753
PS3, Line 753: SetPeerHealth(&config, "D", '-');
> should we also test the (D, '?') case before it goes fully FAILED?
OK, we can do that. But that's not how it's going to be in current implementation for replica health monitoring: it goes from '+' to '-' in the case of a crash, right?
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@754
PS3, Line 754: ASSERT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
> nit: add comment: // We cannot evict because we don't have enough healthy v
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@757
PS3, Line 757: EXPECT_FALSE(CanEvictReplica(config, "A", kReplicationFactor));
: EXPECT_TRUE(IsUnderReplicated(config, kReplicationFactor));
> this duplicates the assertions above
Good catch -- that's left from the conflict resolution.
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@775
PS3, Line 775: // The processs converges: 3 voter replicas, all are healthy.
> nit: it's a little early to say this in this comment. This state would occu
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util-test.cc@779
PS3, Line 779: EXPECT_FALSE(IsUnderReplicated(config, kReplicationFactor));
> perhaps also assert that we cannot evict any nodes here, either.
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc
File src/kudu/consensus/quorum_util.cc:
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc@531
PS3, Line 531: not to evict
> to not evict
Done
http://gerrit.cloudera.org:8080/#/c/8679/3/src/kudu/consensus/quorum_util.cc@544
PS3, Line 544: // * A voter replica may be evicted only if the number of voter replicas in
: // good health without the REPLACE attribute is greater or equal to the
: // specified replication factor.
> is this comment relevant to this patch?
I think it is: since you asked for splitting the patches regarding eviction of voter and non-voter replicas, the corresponding part for the voter replica (which invalidates this comment): https://gerrit.cloudera.org/#/c/8776/
--
To view, visit http://gerrit.cloudera.org:8080/8679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2062c2963fface43062b3154bf5ffa61b4fa684f
Gerrit-Change-Number: 8679
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 03:06:10 +0000
Gerrit-HasComments: Yes