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/08/25 23:54:33 UTC
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Mike Percy has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7842
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
KUDU-2114. Don't re-delete tombstoned replicas
A bug was introduced in 5bca7d8ba185d62952fb3e3163cbe88d20453da0 where
the master will now consider tombstoned replicas to not be deleted, and
will attempt to re-delete them when it gets a tablet report on them.
This patch fixes the problem.
Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
---
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 122 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/7842/1
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:
Line 1170: // The table should have replicas on all three tservers.
"all" three? But there are 4 tservers.
Line 1249: // Ensure that number of delete attempts does not climb for the next several seconds.
Rather than waiting for a number of seconds to allow heartbeats to flow, perhaps you can condition on TSHeartbeat RPC metrics? Then you can stop after some fixed number of heartbeats, potentially speeding up the test too.
http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
I'd rebase on the top of the tree; you'll have to resolve some LOG/VLOG conflicts.
Line 2496: if ((tablet_state == tablet::RUNNING || tablet_state == tablet::FAILED) &&
This effectively keeps out any tablets in INITIALIZED, BOOTSTRAPPING, STOPPING, STOPPED, and SHUTDOWN. I understand keeping out STOPPING since it's such a short term transient state, but why the others? For example, why shouldn't we ask a shut down tablet to delete itself if it's been evicted?
Line 2510: TabletDataState data_state = report.tablet_data_state();
Used in just one place; inline it?
Line 2512: data_state == TABLET_DATA_READY &&
Why enforce TABLET_DATA_READY instead of enforcing "not TABLET_DATA_DELETED and not TABLET_DATA_TOMBSTONED"? Isn't it legal to delete COPYING tablets if they've been evicted?
Line 2526: if (tablet_state == tablet::RUNNING &&
Not getting this one either; does this imply that the reported cstate of a FAILED tablet cannot be trusted?
Line 2537: if (tablet_state == tablet::RUNNING &&
Likewise; we transition to running because of the leader info present in the reported cstate. Should we distrust a FAILED tablet's cstate?
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
TSAN failure is the Gradle daemon dying.
http://gerrit.cloudera.org:8080/#/c/7842/2/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:
PS2, Line 1161: if (!AllowSlowTests()) {
: LOG(WARNING) << "This test sleeps for several seconds and only runs in slow-test mode";
: return;
: }
Still desirable?
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7842/2/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:
PS2, Line 1161: if (!AllowSlowTests()) {
: LOG(WARNING) << "This test sleeps for several seconds and only runs in slow-test mode";
: return;
: }
> Still desirable?
Yeah, the test still takes a while to run. Merging now to stabilize master.
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/integration-tests/delete_table-itest.cc
File src/kudu/integration-tests/delete_table-itest.cc:
Line 1170: // The table should have replicas on all three tservers.
> "all" three? But there are 4 tservers.
Done
Line 1249: // Ensure that number of delete attempts does not climb for the next several seconds.
> Rather than waiting for a number of seconds to allow heartbeats to flow, pe
Done
http://gerrit.cloudera.org:8080/#/c/7842/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
> I'd rebase on the top of the tree; you'll have to resolve some LOG/VLOG con
Done
Line 2496: if ((tablet_state == tablet::RUNNING || tablet_state == tablet::FAILED) &&
> This effectively keeps out any tablets in INITIALIZED, BOOTSTRAPPING, STOPP
I was trying to keep this as similar to the previous logic as possible, which only ran while the tablet was "online". But I don't think the change was necessary, so I'll revert this.
Line 2510: TabletDataState data_state = report.tablet_data_state();
> Used in just one place; inline it?
Done
Line 2512: data_state == TABLET_DATA_READY &&
> Why enforce TABLET_DATA_READY instead of enforcing "not TABLET_DATA_DELETED
I was trying to be conservative, but I think your suggestion is more intuitive and still safe. We won't currently allow deletion of a copying tablet but we may in the future.
Line 2526: if (tablet_state == tablet::RUNNING &&
> Not getting this one either; does this imply that the reported cstate of a
Same as above, however I suppose this change should not be necessary because the cstate should be reliable enough, considering committed config opid is checked. So I'll revert this change.
Line 2537: if (tablet_state == tablet::RUNNING &&
> Likewise; we transition to running because of the leader info present in th
Ah, this check is already performed in ShouldTransitionTabletToRunning(), so it's superfluous. Removing this too.
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
KUDU-2114. Don't re-delete tombstoned replicas
A bug was introduced in 5bca7d8ba185d62952fb3e3163cbe88d20453da0 where
the master will now consider tombstoned replicas to not be deleted, and
will attempt to re-delete them when it gets a tablet report on them.
This patch fixes the problem.
Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
Reviewed-on: http://gerrit.cloudera.org:8080/7842
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 152 insertions(+), 15 deletions(-)
Approvals:
Mike Percy: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2114. Don't re-delete tombstoned replicas
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/7842
to look at the new patch set (#2).
Change subject: KUDU-2114. Don't re-delete tombstoned replicas
......................................................................
KUDU-2114. Don't re-delete tombstoned replicas
A bug was introduced in 5bca7d8ba185d62952fb3e3163cbe88d20453da0 where
the master will now consider tombstoned replicas to not be deleted, and
will attempt to re-delete them when it gets a tablet report on them.
This patch fixes the problem.
Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
---
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 152 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/7842/2
--
To view, visit http://gerrit.cloudera.org:8080/7842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86e5cbe0681557ae86f5bae53f4aeb7635fa6aa6
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>