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 <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 06 Apr 2018 02:58:53 +0000 Gerrit-HasComments: Yes
