You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/09/20 04:49:42 UTC
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8109
to review the following change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leaded did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tserver/heartbeater.cc
6 files changed, 183 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8109/1
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:
PS1, Line 286: cmeta->set_leader_uuid("");
> mayber we should have something like cmeta->clear_leader_uuid() then?
We could do that but I would like to do it in a separate change. I filed KUDU-2150 to track that.
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 56: The
: // master should update its record of which replica is the leader after a new
: // leader is elected.
> Which part of the test asserts this? Would the last WaitForServersToAgree f
correct - the last WaitForServersToAgree() would fail because the master would be unable to add the removed server back, which is missing the record that removed it.
Line 96: TServerDetails* leader = nullptr;
> Nit: don't need to initialize this; it'll always be written to by FindTable
I generally don't like defensive programming but I've been bitten by bad pointers before and I hate leaving pointers around with random stack memory addresses so I typically never do it. But done.
Line 113: // Leader delays heartbeats for 2 sec; followers delay by even longer.
> You already wrote this on L102.
Done
PS1, Line 114: if (cluster_->tablet_server(i)->uuid() != leader->uuid()) {
: followers.push_back(ts_map_[cluster_->tablet_server(i)->uuid()]);
: }
> How about doing this work in the loop on L101?
Done
PS1, Line 124: When it
: // sends a tablet report to the master with the new configuration excluding
: // the removed tablet it will report an unknown leader in the new term.
> Is there a way to assert on this programmatically?
We are asserting on it as a high level (see my comment above) but I don't see an easy way to inspect what the master actually receives. I looked at the logs when developing this test and I can tell you that it generally works as described.
PS1, Line 130: // Wait until the master re-adds the evicted replica and it is back up and
: // running.
: ASSERT_OK(WaitForServersToAgree(kTimeout, ts_map_, tablet_id, 1));
> I might be missing something, but shouldn't it be under the ASSERT_EVENTUAL
It wouldn't buy us much. This function retries.
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
> If the conclusion is that cstate.has_leader_uuid() is untrustworthy, there
Done
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
PS1, Line 384: // Inject latency for testing purposes.
: if (PREDICT_FALSE(FLAGS_heartbeat_inject_latency_before_heartbeat_ms > 0)) {
: TRACE("Injecting $0ms of latency due to --heartbeat_inject_latency_before_heartbeat_ms",
: FLAGS_heartbeat_inject_latency_before_heartbeat_ms);
: SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_inject_latency_before_heartbeat_ms));
: }
> side note we should come up with some reasonable macros for these things
Yeah this is the third time I've done this so it's a pattern. But it's only a few lines of code, so the benefit is marginal.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8109/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS3, Line 152: 2147
> nit: 2147 ?
doh
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 130: Substitute("$0", ms_delay)));
: // Keep track of the followers.
: if (cluster_->tablet_server(i)->uuid() != leader->uuid()) {
> This checks that the last OpIds on all the replicas match.
Thank you for the clarification. For some reason I thought it would be necessary to add that to restore the cluster to the original configuration to start next cycle of the operations under the ASSERT_EVENTUALLY scope, but apparently it's not necessary.
http://gerrit.cloudera.org:8080/#/c/8109/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS3, Line 59: Steps followed in this test:
Thank you for adding the high-level overview for the test scenario.
PS3, Line 152: 1247
nit: 2147 ?
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:
PS1, Line 286: cmeta->set_leader_uuid("");
> We could do that but I would like to do it in a separate change. I filed KU
sgtm, but would really like to see that happen since now we're using this as a special value and that's super clear to the caller. afraid we might get bitten again if we punt on it.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8109/6/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
PS6, Line 82: tablet copy session
not quite right
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8109/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS6, Line 2619: // To ensure consistent handling when modifying or sanitizing the cstate
: // above, copy the whole report on the stack rather than const-casting.
: ReportedTabletPB modified_report = report;
: *modified_report.mutable_consensus_state() = cstate;
isn't this regressing a performance optimization in that it is always now copying the whole report PB, whereas before it was only copying when necessary? worried this will harm scalability.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8109
to look at the new patch set (#4).
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leader did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
12 files changed, 216 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8109/4
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8109
to look at the new patch set (#5).
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leader did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
12 files changed, 216 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8109/5
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8109
to look at the new patch set (#2).
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leaded did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
12 files changed, 191 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8109/2
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 56: The
: // master should update its record of which replica is the leader after a new
: // leader is elected.
> Could you add a comment about this along with that last call to WaitForServ
Done
PS1, Line 130: // Wait until the master re-adds the evicted replica and it is back up and
: // running.
: ASSERT_OK(WaitForServersToAgree(kTimeout, ts_map_, tablet_id, 1));
> Yes, but I thought the cluster configuration should be brought back, so the
This checks that the last OpIds on all the replicas match.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 56: The
: // master should update its record of which replica is the leader after a new
: // leader is elected.
Which part of the test asserts this? Would the last WaitForServersToAgree fail?
Line 96: TServerDetails* leader = nullptr;
Nit: don't need to initialize this; it'll always be written to by FindTabletLeader on success.
Line 113: // Leader delays heartbeats for 2 sec; followers delay by even longer.
You already wrote this on L102.
PS1, Line 114: if (cluster_->tablet_server(i)->uuid() != leader->uuid()) {
: followers.push_back(ts_map_[cluster_->tablet_server(i)->uuid()]);
: }
How about doing this work in the loop on L101?
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
If the conclusion is that cstate.has_leader_uuid() is untrustworthy, there are other calls to it outside catalog_manager.cc. For example, quorum_util.cc, master-path-handlers.cc, ksck.cc, etc. Shouldn't all of them be updated too?
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leader did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Reviewed-on: http://gerrit.cloudera.org:8080/8109
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
12 files changed, 216 insertions(+), 39 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(2 comments)
are you planning on adding a regular unit test or are you pretty confident on your itest?
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/consensus/consensus_meta-test.cc
File src/kudu/consensus/consensus_meta-test.cc:
PS1, Line 286: cmeta->set_leader_uuid("");
mayber we should have something like cmeta->clear_leader_uuid() then?
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
PS1, Line 384: // Inject latency for testing purposes.
: if (PREDICT_FALSE(FLAGS_heartbeat_inject_latency_before_heartbeat_ms > 0)) {
: TRACE("Injecting $0ms of latency due to --heartbeat_inject_latency_before_heartbeat_ms",
: FLAGS_heartbeat_inject_latency_before_heartbeat_ms);
: SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_inject_latency_before_heartbeat_ms));
: }
side note we should come up with some reasonable macros for these things
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8109
to look at the new patch set (#3).
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
This patch fixes a rare, long-standing issue that has existed since at
least 1.4.0, probably much earlier. It appears that the leader election
thread pool changes in 1.5.0 made this problem less rare than it
previously was.
The summary of the issue is that, prior to this fix, it was possible for
the master to believe that no leader existed for a tablet after a
configuration change when in fact a leaded did exist. This could be
triggered if the cluster experiences an election storm in the middle or
right after a configuration change. One workaround for this situation is
to restart the tablet server where the leader replica currently resides.
See KUDU-2147 for an example of the error messages that appear in the
logs when it happens.
In addition to a fix, this patch also includes a regression test that
attempts to exercise a code path likely to trigger the bug.
After the fix, I looped the test 200x with 4 stress threads and it
succeeded 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872165.27701
To verify that the issue was not a regression in 1.5.0, I ran it against
the 1.4 branch and it failed 100% of the time:
http://dist-test.cloudera.org/job?job_id=mpercy.1505872429.29509
Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/quorum_util.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_itest_util.cc
A src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-path-handlers.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
12 files changed, 216 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8109/3
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 124: When it
: // sends a tablet report to the master with the new configuration excluding
: // the removed tablet it will report an unknown leader in the new term.
Is there a way to assert on this programmatically?
PS1, Line 130: // Wait until the master re-adds the evicted replica and it is back up and
: // running.
: ASSERT_OK(WaitForServersToAgree(kTimeout, ts_map_, tablet_id, 1));
I might be missing something, but shouldn't it be under the ASSERT_EVENTUALLY() scope?
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8109/1/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:
PS1, Line 56: The
: // master should update its record of which replica is the leader after a new
: // leader is elected.
> correct - the last WaitForServersToAgree() would fail because the master wo
Could you add a comment about this along with that last call to WaitForServersToAgree() ?
PS1, Line 130: // Wait until the master re-adds the evicted replica and it is back up and
: // running.
: ASSERT_OK(WaitForServersToAgree(kTimeout, ts_map_, tablet_id, 1));
> It wouldn't buy us much. This function retries.
Yes, but I thought the cluster configuration should be brought back, so the removed follower is in the cluster again, if you want to start with the same cluster.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus: KUDU-2147. Unknown leader should not be treated as valid UUID
......................................................................
Patch Set 2: Code-Review+2
Looks good to me but would be good to check in with David/Alexey too.
--
To view, visit http://gerrit.cloudera.org:8080/8109
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie882d05fc58e55836edc0235d14974e65125df6c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No