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/01 03:46:55 UTC
[kudu-CR] [consensus queue] local peer is always healthy
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8709
Change subject: [consensus_queue] local peer is always healthy
......................................................................
[consensus_queue] local peer is always healthy
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume the local peer is always healthy. Otherwise, since the
last_communication_time is not updated by the leader, the leader itself
is not counter as a viable voter.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/1
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc@428
PS5, Line 428: // Only consider a peer to be a viable voter if...
: // ...its last exchange was successful
: viable &= peer->last_exchange_status == PeerStatus::OK;
:
: // ...the peer is up to date with the latest majority.
: //
: // This indicates that it's actively participating in majorities and likely to
: // replicate a config change immediately when we propose it.
: viable &= peer->last_received.index() >= queue_state_.majority_replicated_index;
:
: // ...we have communicated successfully with it recently.
: //
: // This handles the case where the tablet has had no recent writes and therefore
: // even a replica that is down would have participated in the latest majority.
: auto unreachable_time = now - peer->last_communication_time;
: viable &= unreachable_time.ToMilliseconds() < FLAGS_consensus_rpc_timeout_ms;
> My concern is that we had to duplicate the logic of incrementing remaining_
updated
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@337
PS5, Line 337: Substitute("--consensus_rpc_timeout_ms=$0", kTimeout.ToMilliseconds()),
> why do you think this would become flaky with a lower consensus rpc timeout
http://dist-test.cloudera.org//job?job_id=aserbin.1512158682.78553
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@399
PS5, Line 399: SleepFor(kTimeout);
> Why is it necessary to sleep here at all? we already slept at line 383. Sho
That's because we want RPC timeout to pass at this point, and the RPC timeout is set to higher value than 2 * kUnavailableSec.
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 20:21:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#8) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Both test scenarios TestDontEvictIfRemainingConfigIsUnstable_NodesDown
and TestDontEvictIfRemainingConfigIsUnstable_NodesStopped of the
TabletReplacementITest test have been modified to cover the fixed issue.
I verified that commenting out the fix in consensus_queue.cc
makes them fail.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 83 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/8
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8709
to look at the new patch set (#4).
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 49 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/4
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#7) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Both test scenarios TestDontEvictIfRemainingConfigIsUnstable_NodesDown
and TestDontEvictIfRemainingConfigIsUnstable_NodesStopped of the
TabletReplacementITest test have been modified to cover the fixed issue.
I verified that commenting out the fix in consensus_queue.cc
makes them fail.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 82 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/7
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8709
to look at the new patch set (#5).
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Both test scenarios TestDontEvictIfRemainingConfigIsUnstable_NodesDow
and TestDontEvictIfRemainingConfigIsUnstable_NodesStopped of the
TabletReplacementITest test have been modified to cover the fixed issue.
I verified that commenting out the fix in consensus_queue.cc
makes them fail.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 62 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/5
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 7:
I tested the latest rev with 200 iters and 4 stress threads on dist-test and 200/200 passed: http://dist-test.cloudera.org/job?job_id=mpercy.1512162780.112949
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:18:53 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus queue] the leader is always a viable voter
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: [consensus_queue] the leader is always a viable voter
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 04:39:17 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus queue] local peer is always healthy
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: [consensus_queue] local peer is always healthy
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8709/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8709/1//COMMIT_MSG@12
PS1, Line 12: counter
nit: counted
http://gerrit.cloudera.org:8080/#/c/8709/1/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/8709/1/src/kudu/consensus/consensus_queue.cc@420
PS1, Line 420: alive
nit: "local"? or: "This is the local peer, which is guaranteed to be viable."
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 03:52:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [consensus queue] the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: [consensus_queue] the leader is always a viable voter
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8709/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8709/2//COMMIT_MSG@7
PS2, Line 7: healthy
> nit: can you also call it a viable voter here? In case it gets confused wit
Done
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 04:02:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@337
PS5, Line 337: Substitute("--consensus_rpc_timeout_ms=$0", kTimeout.ToMilliseconds()),
> http://dist-test.cloudera.org//job?job_id=aserbin.1512158682.78553
So this test is flaky with 3? Maybe bump back to 5 then?
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 20:31:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:41:48 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc@428
PS5, Line 428: // Only consider a peer to be a viable voter if...
: // ...its last exchange was successful
: viable &= peer->last_exchange_status == PeerStatus::OK;
:
: // ...the peer is up to date with the latest majority.
: //
: // This indicates that it's actively participating in majorities and likely to
: // replicate a config change immediately when we propose it.
: viable &= peer->last_received.index() >= queue_state_.majority_replicated_index;
:
: // ...we have communicated successfully with it recently.
: //
: // This handles the case where the tablet has had no recent writes and therefore
: // even a replica that is down would have participated in the latest majority.
: auto unreachable_time = now - peer->last_communication_time;
: viable &= unreachable_time.ToMilliseconds() < FLAGS_consensus_rpc_timeout_ms;
> This contradicts a bit with the 'prefer short-circuiting the control flow'
My concern is that we had to duplicate the logic of incrementing remaining_viable_voters++ but if you prefer it as-written then I'll leave the decision to your preference since this is pretty subjective.
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@337
PS5, Line 337: Substitute("--consensus_rpc_timeout_ms=$0", kTimeout.ToMilliseconds()),
> Maybe -- my concern was that this test might become flaky in that case. Le
why do you think this would become flaky with a lower consensus rpc timeout?
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@399
PS5, Line 399: SleepFor(kTimeout);
> well, at this point it's enough to sleep for (rpc_timeout - (2 * kUnavalia
Why is it necessary to sleep here at all? we already slept at line 383. Shouldn't we be sleeping for a sufficient period at that point instead in order to trigger the failure detection algorithm?
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 19:57:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:34:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 8: Verified+1
It seems the flake in RaftConsensusITest.TestReplicaBehaviorViaRPC is unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:47:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Both test scenarios TestDontEvictIfRemainingConfigIsUnstable_NodesDown
and TestDontEvictIfRemainingConfigIsUnstable_NodesStopped of the
TabletReplacementITest test have been modified to cover the fixed issue.
I verified that commenting out the fix in consensus_queue.cc
makes them fail.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Reviewed-on: http://gerrit.cloudera.org:8080/8709
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 83 insertions(+), 34 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Andrew Wong: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:41:49 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:33:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc@428
PS5, Line 428: // Only consider a peer to be a viable voter if...
: // ...its last exchange was successful
: viable &= peer->last_exchange_status == PeerStatus::OK;
:
: // ...the peer is up to date with the latest majority.
: //
: // This indicates that it's actively participating in majorities and likely to
: // replicate a config change immediately when we propose it.
: viable &= peer->last_received.index() >= queue_state_.majority_replicated_index;
:
: // ...we have communicated successfully with it recently.
: //
: // This handles the case where the tablet has had no recent writes and therefore
: // even a replica that is down would have participated in the latest majority.
: auto unreachable_time = now - peer->last_communication_time;
: viable &= unreachable_time.ToMilliseconds() < FLAGS_consensus_rpc_timeout_ms;
> perhaps just wrap this part in the if-statement? i.e.
This contradicts a bit with the 'prefer short-circuiting the control flow' rule, but if you think that brings more clarity, then I'll update this.
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@337
PS5, Line 337: Substitute("--consensus_rpc_timeout_ms=$0", kTimeout.ToMilliseconds()),
> can we shorten consensus_rpc_timeout_ms to maybe 3 seconds or 5 seconds?
Maybe -- my concern was that this test might become flaky in that case. Let me try that and run many iterations with dist-test.
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@399
PS5, Line 399: SleepFor(kTimeout);
> is it really necessary to sleep for 15 seconds in this test?
well, at this point it's enough to sleep for (rpc_timeout - (2 * kUnavaliable)). I just put kTimeout for clarity. I'll update this to sleep less.
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 18:50:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 7: Code-Review+2
lmk what you guys think of the test modifications
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:19:15 +0000
Gerrit-HasComments: No
[kudu-CR] [consensus queue] local peer is always healthy
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: [consensus_queue] local peer is always healthy
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8709/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8709/2//COMMIT_MSG@7
PS2, Line 7: healthy
nit: can you also call it a viable voter here? In case it gets confused with Peer "HealthPB" healthy
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 03:57:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8709 )
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
Patch Set 5: Code-Review+1
(3 comments)
looks good, just a couple nits
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/consensus/consensus_queue.cc@428
PS5, Line 428: // Only consider a peer to be a viable voter if...
: // ...its last exchange was successful
: viable &= peer->last_exchange_status == PeerStatus::OK;
:
: // ...the peer is up to date with the latest majority.
: //
: // This indicates that it's actively participating in majorities and likely to
: // replicate a config change immediately when we propose it.
: viable &= peer->last_received.index() >= queue_state_.majority_replicated_index;
:
: // ...we have communicated successfully with it recently.
: //
: // This handles the case where the tablet has had no recent writes and therefore
: // even a replica that is down would have participated in the latest majority.
: auto unreachable_time = now - peer->last_communication_time;
: viable &= unreachable_time.ToMilliseconds() < FLAGS_consensus_rpc_timeout_ms;
perhaps just wrap this part in the if-statement? i.e.
// The local peer is always viable.
if (uuid != local_peer_pb_.permanent_uuid()) {
viable &= ...
...
}
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc
File src/kudu/integration-tests/tablet_replacement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@337
PS5, Line 337: Substitute("--consensus_rpc_timeout_ms=$0", kTimeout.ToMilliseconds()),
can we shorten consensus_rpc_timeout_ms to maybe 3 seconds or 5 seconds?
http://gerrit.cloudera.org:8080/#/c/8709/5/src/kudu/integration-tests/tablet_replacement-itest.cc@399
PS5, Line 399: SleepFor(kTimeout);
is it really necessary to sleep for 15 seconds in this test?
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 01 Dec 2017 08:59:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2230: the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8709
to look at the new patch set (#6).
Change subject: KUDU-2230: the leader is always a viable voter
......................................................................
KUDU-2230: the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Both test scenarios TestDontEvictIfRemainingConfigIsUnstable_NodesDown
and TestDontEvictIfRemainingConfigIsUnstable_NodesStopped of the
TabletReplacementITest test have been modified to cover the fixed issue.
I verified that commenting out the fix in consensus_queue.cc
makes them fail.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
2 files changed, 76 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/6
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus queue] local peer is always healthy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8709
to look at the new patch set (#2).
Change subject: [consensus_queue] local peer is always healthy
......................................................................
[consensus_queue] local peer is always healthy
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/2
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] [consensus queue] the leader is always a viable voter
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8709
to look at the new patch set (#3).
Change subject: [consensus_queue] the leader is always a viable voter
......................................................................
[consensus_queue] the leader is always a viable voter
While iterating over peers in PeerMessageQueue::SafeToEvictUnlocked(),
assume that the leader is always a viable voter. Prior to this patch,
the leader itself was not counted as a viable voter since the leader
does not update its own last_communication_time.
Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
---
M src/kudu/consensus/consensus_queue.cc
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8709/3
--
To view, visit http://gerrit.cloudera.org:8080/8709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id200ec8e562e2b21c8eef09e7b38a8d85d23239c
Gerrit-Change-Number: 8709
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>