Adar Dembo has posted comments on this change. Change subject: KUDU-1607. Unpin tablet flush after failed bootstrap ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/6422/2/src/kudu/integration-tests/tablet_delete-itest.cc File src/kudu/integration-tests/tablet_delete-itest.cc: Line 60: ASSERT_EQ(1, tablet_peers.size()); Hmm, this isn't guaranteed by the time StartCluster() returned? I thought the default behavior for those test methods was to wait until the master has discovered all of the tservers. Line 88: AssertEventually([&] { Why must this be wrapped in AssertEventually? Doesn't WaitForAllBootstrapsToFinish() (or even Restart()) guarantee that the sole peer exists when they're done? http://gerrit.cloudera.org:8080/#/c/6422/2/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 513: meta_->PinFlush(); Nit: add a comment explaining why we pin here (and not, say, earlier). Line 515: WARN_NOT_OK(meta_->UnPinFlush(), Substitute("$0Failed to flush after unpinning", Warning makes sense if some other function below fails, but suppose there's no failure there but there is a failure here; wouldn't we want to return that failure up the call stack? Line 530: Status TabletBootstrap::FinishBootstrap(const string& message, Could make this return void now. -- To view, visit http://gerrit.cloudera.org:8080/6422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id274c6ee1da75bc6f92ab91c0a01edaa009b8962 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
