Mike Percy has posted comments on this change. Change subject: KUDU-1607. Unpin tablet flush after failed bootstrap ......................................................................
Patch Set 4: (3 comments) 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 ta delete_table-test is unique because it's not named -itest, while all the other integration tests are. How about I rename that test to table_delete-itest in a follow up commit? Or you think I should name this delete_tablet-itest? 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 ex This is similar to some ExternalMiniCluster tests, but it shouldn't be too familiar for MiniCluster tests because I just added, in this patch, the capability to easily call the cluster_itest_util.h methods from MiniCluster tests. TBH I don't think we're quite at the point where we can extract a lot of useful stuff yet although it's been on my mind and instead of doing a lot of up-front yak-shaving I have been slowly working around the edges like in this patch. I think in this case it's too early to say this code fragment is very reusable so I'd prefer work on higher-priority things in the short term and punt on this for now. In the medium term I would like to find a way to either pull some functionality into the minicluster base class or a helper that hooks into APIs exposed by the base class to create the tablet server map implicitly. I'm not sure I agree with the workload thing you mentioned, since it's nice to have tests that only do what they need to do. 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() 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: 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
