Mike Percy has posted comments on this change. Change subject: KUDU-1607. Unpin tablet flush after failed bootstrap ......................................................................
Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/6422/2//COMMIT_MSG Commit Message: PS2, Line 7: . > nit: consider dropping the period Hmm. Several people on the team have been writing commit messages on Kudu this way for years, including me. We borrowed this practice from Hadoop commit messages (the JIRA number and trailing period is actually enforced there). I am not sure why I should change now; I think it's really readable. But I'm happy to discuss normalizing commit messages on the dev@ list if you'd like to start that discussion. I'm +1 for consistency across all the commits to the project. http://gerrit.cloudera.org:8080/#/c/6422/2/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: PS2, Line 87: ADD_KUDU_TEST(tablet_delete-itest) > nit: consider keeping the sorted order; it seems this one should come after Done http://gerrit.cloudera.org:8080/#/c/6422/2/src/kudu/integration-tests/mini_cluster.cc File src/kudu/integration-tests/mini_cluster.cc: PS2, Line 344: CHECK_LT(idx, mini_masters_.size()); > nit: this seems redundant because mini_master(int idx) already has correspo Done http://gerrit.cloudera.org:8080/#/c/6422/2/src/kudu/integration-tests/mini_cluster_base.h File src/kudu/integration-tests/mini_cluster_base.h: PS2, Line 31: nanmespace > nit: typo Done 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 t A TabletPeer wraps a Tablet in a single tserver and isn't related to # tservers Line 88: AssertEventually([&] { > Why must this be wrapped in AssertEventually? Doesn't WaitForAllBootstrapsT Restart() no, but you are right that WaitForAllBootstrapsToFinish() is enough and I shouldn't have to AssertEventually() here. PS2, Line 103: ASSERT_EQ(0, tablet_peers.size()); > nit: would ASSERT_TRUE(tablet_peers.empty()) be more idiomatic? Maybe, but if it failed then it wouldn't tell me how many peers it found. So I prefer it this way. 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). I just moved this to be the first thing we do during bootstrap. 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 That's true. It's now a bit uglier, but I fixed this. Line 530: Status TabletBootstrap::FinishBootstrap(const string& message, > Could make this return void now. Done -- 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: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
