You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2018/02/26 18:32:07 UTC
[kudu-CR] WIP KUDU-2293 allow for failed cleanup during copy
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9452
Change subject: WIP KUDU-2293 allow for failed cleanup during copy
......................................................................
WIP KUDU-2293 allow for failed cleanup during copy
WIP: this patch implements a fix by tombstoning tablets that have been
left in TABLET_DATA_COPYING the next time a tablet copy happens. I'm not
convinced yet that this is the best solution. Other implementations
might be:
- if a tablet copy fails and we can't successfully DeleteTabletData
because of on-disks woes, we could at least still update the in-memory
tablet state so we're not left in TABLET_DATA_COPYING. Note: we'll be
left in a state where in-memory and on-disk state disagree with each
other. Maybe that's okay.
- if we fail to update the metadata state, crash. There will be no
inconsistencies because the next time the server starts up, tablets
left in TABLET_DATA_COPYING will be tombstoned.
If a tablet copy were to fail due to a lack of space, both the on-disk
and in-memory tablet metadata are left in TABLET_DATA_COPYING.
Broken invariant: a tablet that is transitioned to a state that we only
expect during transitions (TABLET_DATA_COPYING) is left in that state
when the transition fails. As such, something in the transition map as
not copying is actually seen as copying, even if it's not.
This is because the invariant we try to uphold is that the on-disk state
is identical to the in-memory state. When the disk is unable to keep up
with memory (e.g. because a disk is full), special care must be taken to
ensure either that we can handle the breakage of either invariant, or
that we don't break either.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
2 files changed, 90 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/1
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/8/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/8/src/kudu/tserver/tablet_copy_client.cc@60
PS8, Line 60: #include "kudu/util/monotime.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 03 Apr 2018 03:22:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc@302
PS5, Line 302: // Also validate the term of the source peer, in case they are
> I feel like this class would be easier to follow if we just always called A
Yeah, that's fair. I'm going to go with kInitialized, kStarting, kStarted, kFinishing, and kFinished.
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc@459
PS5, Line 459: //
> Hmm, why load now and not replace anymore? Doesn't this make it more likely
Great question, this wasn't really clear: with LoadFromSuperBlock instead of Replace, the only call that can fail due to a disk error is DeleteTabletData(), which flushes anyway. I'll update the comments.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 21:42:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@455
PS10, Line 455: //
> OK, let's add a comment here to the following effect:
Done
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: // delete the tablet's data dir group, WAL segments, etc.
> OK, because we set tablet_data_state_ = delete_type under the lock before w
Done
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc@607
PS10, Line 607: TabletServerErrorPB::INVALID_CONFIG);
> We're not doing that revert, but we hypothetically could do that if we want
Ack
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 01:15:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(4 comments)
I updated this a little based on our offline discussion, which raised a few points:
- The checks for meta_ seemed redundant with kStarting. I moved the placement of kStarting and now the meta_ is guaranteed to be non-null if in kStarting
- The distinction between kFinishing and kFinished weren't super clear. The main difference from a programming perspective is that for kFinished to be reached, either Finish() or Abort() must have run successfully. Calling Abort() in kFinishing isn't a no-op as it is while in kFinish.
- There are some things left that don't have great semantics. E.g. this patch still depends on the *lack* of TSTabletManager::DeleteTabletData's idempotence.
Also removed the FATAL as you suggested.
To be continued, but worth a look to see if these updates make it more digestible.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@405
PS9, Line 405:
: boost::optional<OpId> last_logged_opid = superblock_->tombstone_last_logged_opid();
: auto revert_activate_superblock = MakeScopedCleanup([&] {
: // If we fail below, revert the updated state so further calls to Abort()
: // can clean up appropriately.
: if (last_logged_opid) {
: *superblock_->mutable_tombstone_last_logged_opid() = *last_logged_opid;
: }
: superblock_->set_tablet_data_state(TabletDataState::TABLET_DATA_COPYING);
: });
:
: sup
> nit: this is a minor point but I think this would be a little easier to fol
Done
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@448
PS9, Line 448: // SetMetadataState() and FlushMetadata() calls or something.
> Looks like we can move this to the top of the function here since it will n
Done
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@454
PS9, Line 454: // tablet, all the partial blocks we persisted will be deleted.
> Let's also put this before the SCOPED_CLEANUP.
Done
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@455
PS9, Line 455: WARN_NOT_OK(meta_->LoadFromSuperBlock(*superblock_),
> isn't it possible to violate the CHECK_EQ in the SCOPED_CLEANUP if this fai
Done
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 06 Apr 2018 02:58:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#11).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
FATAL error if another tablet copy were started for the same replica, as
the server would attempt to copy over a replica with data already marked
TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Another is added to tablet_copy_client-test that tests getting into the
various states in the updated tablet copy state machine.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
6 files changed, 270 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/11
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> Gotcha. What do you think about changing the implementation to the below an
I agree with removing the revert logic, but I think the call to DeleteDataDirGroup() should go before the Flush(), in case the Flush() fails due to a disk error or something, following the guidance of "leave things dead in memory even if we fail to leave them dead on disk"?
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 10 Apr 2018 21:06:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@213
PS10, Line 213:
> Hmm good question. I suppose the argument for not including it here is that
OK. So I our guarantees are the following, which we should document in the header file I think:
- if we have made any changes to disk, we are no longer in kInitialized state (we'll be in kStarting or better)
- if we are in kStarting state or higher then meta_ is guaranteed to be set, but meta_ may actually be set while still in kInitialized state
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@325
PS10, Line 325: state_ = kStarting;
> Responded up there, but to add another point, that would mean that any time
Ah, good catch about the last_logged_opid. OK, let's leave it as-is.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@455
PS10, Line 455: WARN_NOT_OK
> Hmm. I wouldn't be opposed to making this a CHECK. I made it WARN_NOT_OK()
OK, let's add a comment here to the following effect:
// We warn instead of return early here upon load failure because even if the superblock protobuf was somehow corrupted, we still want to attempt to delete the tablet's data dir group and WAL segments.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> Ah, good callout. This was kind of hacky and more in-line with the invarian
OK, so we call TSTabletManager::DeleteTabletData() which calls TabletMetadata::DeleteTabletData() which calls DeleteDataDirGroup(tablet_id). What I think you're saying is that TabletMetadata::DeleteTabletData() reverts deletion of the data dir group if TabletMetadata::Flush() fails, so in order to counteract that revert we are preemptively deleting it so that it becomes impossible to revert in such a case. A couple questions for you:
1. What happens if we fail to remove that entry?
2. What happens if we remove that entry but not all of the blocks got deleted by Flush()?
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 09 Apr 2018 19:33:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h@148
PS9, Line 148: kStarting
> Since we are still conditioning on meta_, I don't see the purpose of this n
meta_ gets set in SetTabletToReplace(), which happens before Start(). While in this state, meta_ may have been updated, and its state will be TABLET_DATA_COPYING, and so if we Abort() while in this state, it needs to be cleaned up.
The difference between this and kStarted is that kStarted indicates that Start() has completed completely. E.g. it'd be a programming error to start copying data while in kStarting, but it's correct to do so while in kStarted.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h@157
PS9, Line 157: kFinishing
> What is the purpose of this state? Do we need it?
We could do away with this and replace setting kFinishing with kFinished in Finish(). If we do that, then kFinished won't actually mean "finished" -- it'll mean "we've finished copying everything over and may still need to set the data to TABLET_DATA_READY". Maybe that's ok. WDYT?
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@440
PS9, Line 440: CHECK_EQ(tablet::TABLET_DATA_TOMBSTONED
> what guarantees this?
Success or fail, DeleteTabletData() leaves the tablet in TABLET_DATA_TOMBSTONED. Maybe we could split that into some `meta_->SetTabletState()` that would make the state change more explicit and a `DeleteTabletData()`.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 06 Apr 2018 01:34:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 7:
The last failure was actually related to the change. In patch set 6, I'd moved the copy state machine to kStarting a bit too early, even based on my own descriptions of the states.
The result was that the replica's metadata was incorrectly overwritten (the copy would end early because to an invalid term, and proceed to Abort() and overwrite the tablet metadata with the invalid superblock!) I've updated the placement of the state machine to match the descriptions, which avoids this.
I looped TestRejectRogueLeader 200 times to ensure it doesn't run into the issues (before it would happen ~15/200).
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 03 Apr 2018 01:51:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@155
PS10, Line 155: kFinishing,
If we're going to add states we should add tests to verify their functionality, maybe in tablet_copy_client-test.cc
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@213
PS10, Line 213:
Based on the latest invariant change should we add the following here?
state_ = kStarting;
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@325
PS10, Line 325: state_ = kStarting;
Maybe we should move this to line 213 per my comment above?
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@446
PS10, Line 446: we call DeleteTabletData()
ok but see below on my question about WARN_NOT_OK
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@455
PS10, Line 455: WARN_NOT_OK
Why WARN_NOT_OK and not RETURN_NOT_OK? Or should failure here be fatal, since this is metadata? I guess what's going to happen if this fails for some reason is that we are going to leak some blocks.
If that's the approach we should comment why we are choosing to ignore errors in this step.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
hmm, should we make this part of TSTabletManager::DeleteTabletData() ?
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc@607
PS10, Line 607: Status s = DeleteTabletData(meta, cmeta_manager_, TABLET_DATA_TOMBSTONED,
Interesting. I guess this is the alternative to what you're done in tablet_copy_client.cc.
After considering separating meta->DeleteData() from this stuff, it seems strange to revert TABLET_DATA_TOMBSTONED back to READY or something if some blocks couldn't be accessed for some reason. So I'm not sure we would want to revert a failed DeleteData(). Therefore it seems preferable to drop the approach in this file and stick to the other one in tablet_copy_client.cc. Let me know what you think.
If we keep this one: We should call old_replica->Shutdown() before doing anything with the data files and also add a test.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 07 Apr 2018 00:59:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tablet/tablet_metadata.cc@218
PS11, Line 218: // Unregister the tablet's data dir group in memory (it is stored on disk in
> nit: This isn't a very useful comment. Mind adding a comment more along the
Done
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.h@153
PS11, Line 153: receiving WAL segments or blocks from the tabl
> this is a little confusing and not really true, because the metadata on dis
Done
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.cc@400
PS11, Line 400:
> Does the kFinishing state serve any functional purpose? We are already guar
You are absolutely right
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 02:10:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc@302
PS5, Line 302: cleanup_cb_ = function<void(void)>([&] {
I feel like this class would be easier to follow if we just always called Abort() in the destructor like before and keep track of whether we're kStarting or better, or something like that. What do you think? It seems like part of the problem in this class is that the values of the state_ enum aren't really sufficient for all cases.
http://gerrit.cloudera.org:8080/#/c/9452/5/src/kudu/tserver/tablet_copy_client.cc@459
PS5, Line 459: RETURN_NOT_OK(meta_->LoadFromSuperBlock(*superblock_));
Hmm, why load now and not replace anymore? Doesn't this make it more likely to orphan blocks on crash?
Then again, we are about to flush the superblock in DeleteTabletData(), so maybe it's not required.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 08:55:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#9).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 169 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/9
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#5).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
Additionally, we previously used the tablet copy internal state machine
to determine whether to call Abort(). This wasn't always right, since
the state machine updates when various stages complete, rather than when
various stages start, and w.r.t. cleanup, it's useful to know when we
have already begun to update state on disk, and when we have finished.
So in addition to the above changes to Abort(), this patch also
registers a function to be called by TabletCopyClient's destructor that
calls Abort() if cleanup is necessary (i.e. if a tablet copy has started
and not completed).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 157 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/5
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] WIP: KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#4).
Change subject: WIP: KUDU-2293 fix cleanup after failed copies
......................................................................
WIP: KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and the tablet would remain in
TABLET_DATA_COPYING, both in-memory and on-disk. This would lead to a
CHECK failure the if another tablet copy were started for the same
replica, as server would attempt to copy over a replica with data
already marked TABLET_DATA_COPYING.
The above behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
Additionally, we previously used the tablet copy internal state machine
to determine whether to call Abort(). This wasn't always right, since
the state machine updates when various stages complete, rather than when
various stages start, and w.r.t. cleanup, it's useful to know when we
have already begun to update state on disk, and when we have finished.
So in addition to the above changes to Abort(), this patch also
registers a function to be called by TabletCopyClient's destructor that
calls Abort() if cleanup is necessary (i.e. if a tablet copy has started
and not completed).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 159 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/4
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 02:14:13 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(4 comments)
Just responding to your question
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@140
PS10, Line 140: // The copy has yet to do anything.
> Let's add something to the effect of: No changes have yet been made on disk
Done
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@143
PS10, Line 143: updating metadata
> Edit: update metadata on disk
Done
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@213
PS10, Line 213:
> OK. So I our guarantees are the following, which we should document in the
Done
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> OK, so we call TSTabletManager::DeleteTabletData() which calls TabletMetada
1. If we don't remove the data dir group, if we ever try to create a new data dir group thereafter, the caller would get an AlreadyPresent error. Right now, removal of data dir groups can't fail (if the group is already removed, it's a no-op).
2. I think it'd only be really wrong to remove a tablet's entry is if we _actually_ wanted to continue writing new blocks to that tablet. I.e. if we for some reason delete the tablet's data dir group and then call GetNextBlock() for that tablet, the caller would get an error. In the case of deleting tablet data, I think the assumption is that the tablet is being or has been shut down, so we wouldn't use the data dir group anyway. In the context of a tablet copy, I think we're guaranteed to be the only writer to the replica, and upon Abort()ing, we wouldn't write any more blocks to that tablet.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 09 Apr 2018 22:09:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 6:
All of the failures seem to be caused by clock sync issues.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 22:48:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 9:
(7 comments)
This is a valiant effort however due to the complexity maybe you were right all along. We should try to clean up as well as we can, no doubt about it. However maybe we should also remove the FATAL in ts_tablet_manager.cc since it's such a tricky invariant to maintain.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h@148
PS9, Line 148: kStarting
Since we are still conditioning on meta_, I don't see the purpose of this new state.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.h@157
PS9, Line 157: kFinishing
What is the purpose of this state? Do we need it?
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@405
PS9, Line 405: superblock_->set_tablet_data_state(tablet::TABLET_DATA_READY);
:
: boost::optional<OpId> last_logged_opid = superblock_->tombstone_last_logged_opid();
: superblock_->clear_tombstone_last_logged_opid();
: auto revert_activate_superblock = MakeScopedCleanup([&] {
: // If we fail below, revert the updated state so further calls to Abort()
: // can clean up appropriately.
: if (last_logged_opid) {
: *superblock_->mutable_tombstone_last_logged_opid() = *last_logged_opid;
: }
: superblock_->set_tablet_data_state(TabletDataState::TABLET_DATA_COPYING);
: });
nit: this is a minor point but I think this would be a little easier to follow written like this, by setting up the revert before changing the state:
LOG_WITH_PREFIX(INFO) << "Tablet Copy complete. Replacing tablet superblock.";
SetStatusMessage("Replacing tablet superblock");
boost::optional<OpId> last_logged_opid = superblock_->tombstone_last_logged_opid();
auto revert_activate_superblock = MakeScopedCleanup([&] {
// If we fail below, revert the updated state so further calls to Abort()
// can clean up appropriately.
if (last_logged_opid) {
*superblock_->mutable_tombstone_last_logged_opid() = *last_logged_opid;
}
superblock_->set_tablet_data_state(TabletDataState::TABLET_DATA_COPYING);
});
superblock_->clear_tombstone_last_logged_opid();
superblock_->set_tablet_data_state(tablet::TABLET_DATA_READY);
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@440
PS9, Line 440: CHECK_EQ(tablet::TABLET_DATA_TOMBSTONED
what guarantees this?
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@448
PS9, Line 448: if (!meta_ || state_ == kInitialized || state_ == kFinished) {
Looks like we can move this to the top of the function here since it will never overlap with the scoped cleanup above.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@454
PS9, Line 454: DCHECK_EQ(tablet::TABLET_DATA_COPYING, superblock_->tablet_data_state());
Let's also put this before the SCOPED_CLEANUP.
http://gerrit.cloudera.org:8080/#/c/9452/9/src/kudu/tserver/tablet_copy_client.cc@455
PS9, Line 455: RETURN_NOT_OK(meta_->LoadFromSuperBlock(*superblock_));
isn't it possible to violate the CHECK_EQ in the SCOPED_CLEANUP if this fails, say because we can't load or create a data dir group?
If so, it seems we should remove that CHECK_EQ since those are generally intended for programming errors.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 06 Apr 2018 01:09:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tablet/tablet_metadata.cc@218
PS11, Line 218: // Remove the tablet's data dir group tracked by the DataDirManager.
nit: This isn't a very useful comment. Mind adding a comment more along the lines of what I suggested in a comment earlier?
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.cc@400
PS11, Line 400: state_ = kFinishing;
Does the kFinishing state serve any functional purpose? We are already guaranteed, based on the CHECK above, that we were in kStarted state when entering this function, so Abort() will execute its cleanup code either way.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 01:46:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@155
PS10, Line 155: kFinishing,
> If we're going to add states we should add tests to verify their functional
Ack
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@213
PS10, Line 213:
> Based on the latest invariant change should we add the following here?
Hmm good question. I suppose the argument for not including it here is that no changes are being made to the metadata, and therefore there's nothing to role back. That argument assumes that all the changes being done to meta_ are done through this class, which seems true.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@325
PS10, Line 325: state_ = kStarting;
> Maybe we should move this to line 213 per my comment above?
Responded up there, but to add another point, that would mean that any time between calling SetTabletToReplace() and here (which basically means any potential point of failure in Start()) will trigger the cleanup in Abort(), and right now, Abort() entails loading superblock_ and flushing it.
Also note that only at L320 do we set superblock_'s last logged opid, and flushing the superblock without keeping the last logged opid seems like an error.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@446
PS10, Line 446: we call DeleteTabletData()
> ok but see below on my question about WARN_NOT_OK
Ack
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@455
PS10, Line 455: WARN_NOT_OK
> Why WARN_NOT_OK and not RETURN_NOT_OK? Or should failure here be fatal, sin
Hmm. I wouldn't be opposed to making this a CHECK. I made it WARN_NOT_OK() because:
1. it guarantees we get to DeleteTabletData, as you pointed out
2. it doesn't seem like it should be a blocker to getting to DeleteTabletData, even if it does throw an error. I suppose it could maybe indicate a programming error in how we load from PB?
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> hmm, should we make this part of TSTabletManager::DeleteTabletData() ?
Ah, good callout. This was kind of hacky and more in-line with the invariant that we should try to keep on-disk and in-memory consistent. If you take a look at TabletMetadata::DeleteTabletData(), we'll revert the deletion of a data dir group on error to uphold that invariant (although we clearly don't do a complete job in that function because the in-memory state is still left deleted). Maybe we should _not_ revert that group deletion, in which case this wouldn't be necessary.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc@607
PS10, Line 607: Status s = DeleteTabletData(meta, cmeta_manager_, TABLET_DATA_TOMBSTONED,
> Interesting. I guess this is the alternative to what you're done in tablet_
Yeah, I wouldn't be opposed to removing this; wasn't sure how to best remove the LOG(FATAL). E.g. is it sufficient to just treat it as tombstoned and pas through here?
Although I think I might be missing something. Where are we reverting TABLET_DATA_TOMBSTONED back to READY?
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 07 Apr 2018 01:29:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 12:
IWYU is telling me to remove "kudu/gutil/callback.h" from tablet/tablet_metadata.h, which breaks the build.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 02:11:06 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@143
PS10, Line 143: updating metadata
> updating metadata
Edit: update metadata on disk
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 09 Apr 2018 19:36:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: WIP: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9452/2//COMMIT_MSG@39
PS2, Line 39: This patch changes them to be:
: - kInitialized: Start() may have been called, but has not attempted to
: update anything on-disk
: - kStarted: Start() has been called and has updated on-disk state,
: Finish() has not completed successfully
: - kFinished: Finish() has been called and completed successfully
Might be cleaner to encapsulate these semantics into a cancelable cleanup callback, and keep the original semantics.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Sat, 10 Mar 2018 00:16:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> 1. If we don't remove the data dir group, if we ever try to create a new da
Gotcha. What do you think about changing the implementation to the below and get rid of other calls to delete the data dir group? I believe that will ensure that if we manage to get the tombstoned superblock to disk then we will unregister the data dir group, but not otherwise.
diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc
index 004a5ed..bc90bf8 100644
--- a/src/kudu/tablet/tablet_metadata.cc
+++ b/src/kudu/tablet/tablet_metadata.cc
@@ -215,22 +215,14 @@ Status TabletMetadata::DeleteTabletData(TabletDataState delete_type,
}
}
- // Keep a copy of the old data dir group in case of flush failure.
- DataDirGroupPB pb;
- bool old_group_exists = fs_manager_->dd_manager()->GetDataDirGroupPB(tablet_id_, &pb).ok();
-
- // Remove the tablet's data dir group tracked by the DataDirManager.
- fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
- auto revert_group_cleanup = MakeScopedCleanup([&]() {
- if (old_group_exists) {
- fs_manager_->dd_manager()->LoadDataDirGroupFromPB(tablet_id_, pb);
- }
- });
-
// Flushing will sync the new tablet_data_state_ to disk and will now also
// delete all the data.
RETURN_NOT_OK(Flush());
- revert_group_cleanup.cancel();
+
+ // Unregister the tablet's data dir group from memory (its configuration is
+ // stored in the superblock).
+ // If the data dir group was somehow already deleted, this is a no-op.
+ fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
// Re-sync to disk one more time.
// This call will typically re-sync with an empty orphaned blocks list
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 10 Apr 2018 20:55:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@140
PS10, Line 140: // The copy has yet to do anything.
Let's add something to the effect of: No changes have yet been made on disk.
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.h@143
PS10, Line 143: updating metadata
updating metadata
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 09 Apr 2018 19:35:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/tablet_copy_client.cc@458
PS10, Line 458: fs_manager_->dd_manager()->DeleteDataDirGroup(tablet_id_);
> I agree with removing the revert logic, but I think the call to DeleteDataD
OK, because we set tablet_data_state_ = delete_type under the lock before we Flush(). +1 from me.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 00:52:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#3).
Change subject: WIP: KUDU-2293 fix cleanup after failed copies
......................................................................
WIP: KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and the tablet would remain in
TABLET_DATA_COPYING, both in-memory and on-disk. This would lead to a
CHECK failure the if another tablet copy were started for the same
replica, as server would attempt to copy over a replica with data
already marked TABLET_DATA_COPYING.
The above behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
The following semantic changes are made to help clarify cleanup for
tablet copies. Before, the semantics for tablet copy states were:
- kInitialized: Start() has not completed successfully
- kStarted: Started() has completed successfully, Finish() has not
been called
- kFinished: Finish() has been called, but may not have finished
This patch changes them to be:
- kInitialized: Start() may have been called, but has not attempted to
update anything on-disk
- kStarted: Start() has been called and has updated on-disk state,
Finish() has not completed successfully
- kFinished: Finish() has been called and completed successfully
An test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/ts_tablet_manager.cc
5 files changed, 144 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/3
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] WIP: KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#2).
Change subject: WIP: KUDU-2293 fix cleanup after failed copies
......................................................................
WIP: KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and the tablet would remain in
TABLET_DATA_COPYING, both in-memory and on-disk. This would lead to a
CHECK failure the if another tablet copy were started for the same
replica, as server would attempt to copy over a replica with data
already marked TABLET_DATA_COPYING.
The above behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
The following semantic changes are made to help clarify cleanup for
tablet copies. Before, the semantics for tablet copy states were:
- kInitialized: Start() has not completed successfully
- kStarted: Started() has completed successfully, Finish() has not
been called
- kFinished: Finish() has been called, but may not have finished
This patch changes them to be:
- kInitialized: Start() may have been called, but has not attempted to
update anything on-disk
- kStarted: Start() has been called and has updated on-disk state,
Finish() has not completed successfully
- kFinished: Finish() has been called and completed successfully
An test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/ts_tablet_manager.cc
5 files changed, 139 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/2
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
FATAL error if another tablet copy were started for the same replica, as
the server would attempt to copy over a replica with data already marked
TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine didn't accurately reflect when cleanup was necessary (e.g.
the copy would be set to kStarted only at the end of Start(), so if
Abort() were called due to a failure in Start(), no cleanup was done).
This patch updates the state machine to account for this by introducing a
new state kStarting that indicates that there exists state to clean up
even if Start() has not completed, and by moving the setting of
kFinished such that cleanup is done even if Finish() fails.
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Another is added to tablet_copy_client-test that tests getting into the
various states in the updated tablet copy state machine.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Reviewed-on: http://gerrit.cloudera.org:8080/9452
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
6 files changed, 271 insertions(+), 41 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Andrew Wong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/11/src/kudu/tserver/tablet_copy_client.h@153
PS11, Line 153: receiving anything from the tablet copy source
this is a little confusing and not really true, because the metadata on disk is from the tablet copy source
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 01:53:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#10).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/ts_tablet_manager.cc
5 files changed, 179 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/10
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#8).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 170 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/8
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
http://gerrit.cloudera.org:8080/#/c/9452/10/src/kudu/tserver/ts_tablet_manager.cc@607
PS10, Line 607: Status s = DeleteTabletData(meta, cmeta_manager_, TABLET_DATA_TOMBSTONED,
> Yeah, I wouldn't be opposed to removing this; wasn't sure how to best remov
We're not doing that revert, but we hypothetically could do that if we wanted it to be "atomic". I just don't think it really make sense because we could have already deleted some blocks or something like that.
My point is that once we try to DeleteData() then I think we are stuck leaving the metadata in whatever tablet data state we specified there, regardless of what make it to disk or not. Try to provide a "cleaner" guarantee for the API would be a path to madness. :)
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 07 Apr 2018 01:44:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#12).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
FATAL error if another tablet copy were started for the same replica, as
the server would attempt to copy over a replica with data already marked
TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine didn't accurately reflect when cleanup was necessary (e.g.
the copy would be set to kStarted only at the end of Start(), so if
Abort() were called due to a failure in Start(), no cleanup was done).
This patch updates the state machine to account for this by introducing a
new state kStarting that indicates that there exists state to clean up
even if Start() has not completed, and by moving the setting of
kFinished such that cleanup is done even if Finish() fails.
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Another is added to tablet_copy_client-test that tests getting into the
various states in the updated tablet copy state machine.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
6 files changed, 271 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/12
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#7).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 169 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/7
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 12: Verified+1
The test failures seem to be unrelated (tsan reporting a race in the dns resolver). Also mentioned the IWYU fluke.
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Apr 2018 02:45:17 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9452 )
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9452/7/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/9452/7/src/kudu/tserver/tablet_copy_client.h@20
PS7, Line 20: #include <memory>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/9452/7/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/9452/7/src/kudu/tserver/tablet_copy_client.cc@60
PS7, Line 60: #include "kudu/util/scoped_cleanup.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 03 Apr 2018 03:00:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2293 fix cleanup after failed copies
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9452
to look at the new patch set (#6).
Change subject: KUDU-2293 fix cleanup after failed copies
......................................................................
KUDU-2293 fix cleanup after failed copies
Before, when a tablet server failed to tablet copy, Kudu would perform a
best-effort cleanup of the partially-copied replica and leave the tablet
tombstoned. If this tombstoning were to fail due to disk issues (e.g.
out of space), Kudu would allow this and tablet would remain in
TABLET_DATA_COPYING both in-memory and on-disk. This would lead to a
CHECK failure if another tablet copy were started for the same replica,
as the server would attempt to copy over a replica with data already
marked TABLET_DATA_COPYING.
This behavior arose from trying to balance two invariants:
- keep on-disk state consistent with in-memory state when possible
- when a tablet copy fails, leave it in as dead of a state as we can
(i.e. TABLET_DATA_TOMBSTONED with no transitions in progress)
This patch updates the Abort() logic to lean towards the latter
invariant: if a tablet copy fails, at least its in-memory state will be
set as TABLET_DATA_TOMBSTONED. This may not be true on-disk, but that's
okay because either 1) the tablet server will eventually overwrite it
via another tablet copy (at which time its data must _not_ be in the
TABLET_DATA_COPYING state), or 2) the server will be restarted and the
tablet will be tombstoned upon seeing a non-TABLET_DATA_READY state
anyway.
We use the tablet copy internal state machine to determine whether to do
anything during Abort(). This wasn't always right before, since the
state machine would only update at the completion of various methods
(e.g. Start()), which didn't necessarily reflect whether or not clean
up was necessary. This patch updates the state machine to be slightly
more fine-grained, introducing new states kStarting and kFinishing that
indicate that there exists state to clean up (even if, e.g., Start() has
not completed successfully).
A test is added to tablet_copy-itest that tests failures to copy,
ensuring that the tablet is left in such an state that further copies
are possible without crashing. The test uses EIO injection to fail the
copies, but the logic is the same as if full disks were used instead.
Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
4 files changed, 166 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/9452/6
--
To view, visit http://gerrit.cloudera.org:8080/9452
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0459d484a3064aa2de392328e2910c9f6741be04
Gerrit-Change-Number: 9452
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>