David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1607. Unpin tablet flush after failed bootstrap ......................................................................
Patch Set 4: (6 comments) sorry had some comments on an old rev. I checked and most still seem relevant though http://gerrit.cloudera.org:8080/#/c/6422/3/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: PS3, Line 230: std::shared_ptr<rpc::Messenger> messenger() const override; : std::shared_ptr<master::MasterServiceProxy> master_proxy() const override; : std::shared_ptr<master::MasterServiceProxy> master_proxy(int idx) const override; does the superclass have the docs? http://gerrit.cloudera.org:8080/#/c/6422/3/src/kudu/integration-tests/mini_cluster_base.h File src/kudu/integration-tests/mini_cluster_base.h: PS3, Line 77: // Return a messenger for use by clients. It does :), great http://gerrit.cloudera.org:8080/#/c/6422/3/src/kudu/integration-tests/tablet_delete-itest.cc File src/kudu/integration-tests/tablet_delete-itest.cc: PS3, Line 38: class TabletDeleteITest : public MiniClusterITestBase nit: it's weird that we have a delete_table-test (DeleteTableTest) and a tablet_delete-itest (TabletDeleteITest). Could you make the naming consistent? PS3, Line 43: NO_FATALS(StartCluster(1)); // 1 TS. : TestWorkload workload(cluster_.get()); : workload.set_num_replicas(1); : workload.Setup(); : : std::unordered_map<std::string, itest::TServerDetails*> ts_map; : ValueDeleter del(&ts_map); : ASSERT_OK(itest::CreateTabletServerMap(cluster_->master_proxy().get(), : cluster_->messenger(), : &ts_map)); : auto* mts = cluster_->mini_tablet_server(0); : auto* ts = ts_map[mts->uuid()]; : : scoped_refptr<TabletPeer> tablet_peer; : AssertEventually([&] { : vector<scoped_refptr<TabletPeer>> tablet_peers; : mts->server()->tablet_manager()->GetTabletPeers(&tablet_peers); : ASSERT_EQ(1, tablet_peers.size()); : tablet_peer = tablet_peers[0]; : }); : NO_FATALS(); : : workload.Start(); : while (workload.rows_inserted() < 100) { : SleepFor(MonoDelta::FromMilliseconds(10)); : } : workload.StopAndJoin(); This code seems eerly familiar from a previous patch of yours. Could you extract a helper? I mean to start an n node cluster, make sure it has n tablet peers. Maybe even add a bool to write something for a while, since this is pretty common setup. http://gerrit.cloudera.org:8080/#/c/6422/3/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 472: Status unpin_status = meta_->UnPinFlush(); > Not if we don't flush the superblock. Unpinning, even for a failed bootstrap should be ok. If it failed it's not like we're using a part of the data or trying to fix it so from a correctness perspective it should be ok to unpin. http://gerrit.cloudera.org:8080/#/c/6422/4/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: PS4, Line 465: tablet_ && log_ could we just have bootstrap_status.ok() -- 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: 4 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
