Andrew Wong has posted comments on this change. Change subject: mini_cluster: support multiple data dirs ......................................................................
Patch Set 14: (7 comments) http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/integration-tests/internal_mini_cluster.cc File src/kudu/integration-tests/internal_mini_cluster.cc: PS11, Line 60: opts_(st > great - seems alright to me Done http://gerrit.cloudera.org:8080/#/c/7211/13/src/kudu/integration-tests/internal_mini_cluster.cc File src/kudu/integration-tests/internal_mini_cluster.cc: Line 58: InternalMiniCluster::InternalMiniCluster(Env* env, InternalMiniClusterOptions options) > warning: pass by value and use std::move [modernize-pass-by-value] Done Line 60: opts_(std::move(options)), > warning: parameter 'options' is passed by value and only copied once; consi Done http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: Line 60: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root_, &created)); > Are we sure this step is required now but not previously? Yeah, we require that the parent directory of any data dirs exists at startup time Line 102: Shutdown(); > Shutdown() still here in Restart() Ah thanks http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/mini_tablet_server.cc File src/kudu/tserver/mini_tablet_server.cc: Line 65: CHECK_OK(env_util::CreateDirIfMissing(Env::Default(), fs_root, &created)); > See comment in mini_master.cc Done http://gerrit.cloudera.org:8080/#/c/7211/14/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: Line 103: tablet_replica_.reset(); > Why is this needed now but not before? If StartTabletServer is before the tablet_replica_ is reset (either this way or with ShutdownTablet()), the dtor for the server will fail a CHECK because there's still exists a reference to some refcounted data (the tablet_replica_). When the multidir test was in tablet_server-test, that's what happened, but it isn't the case anymore. Alternatively, we can enforce that no server is running (like in mini_master) and add multidir support to ShutdownAndRebuildTablet() too, which is probably the right thing to do. -- To view, visit http://gerrit.cloudera.org:8080/7211 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I52c9352f1a3565d58149cf2c63d37246c6b39c23 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
