Andrew Wong has posted comments on this change. Change subject: mini_cluster: support multiple data dirs ......................................................................
Patch Set 11: (14 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: fs_root_ > Can we get rid of this member too? We could put this in the body of the con Done, but it came at the cost of making opts_ non-const. http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: Line 1349: TEST_F(MasterTest, TestMultiDirMaster) { > If this is just unit test coverage for the MiniMaster I think it should be Done http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: PS11, Line 59: data$0 > I think it would be more readable as "data-$0" Done Line 70: void MiniMaster::SetMasterAddresses(const vector<HostPort>& master_addrs) { > nit: add CHECK(!master_) so this is only called when it's not running Done Line 71: opts_.master_addresses = master_addrs; > nit: pass by value and use std::move() here Done PS11, Line 85: if (opts_.master_addresses.empty()) { : return master_->WaitForCatalogManagerInit(); : } > Add comment: // Wait for catalog manager to be ready if we only have a sing Done Line 92: opts_.rpc_opts.rpc_bind_addresses = bound_rpc_.ToString(); > add: CHECK(!master_) so this is only called after Shutdown() Done PS11, Line 93: HostPort(bound_http_).host() > no need to construct a HostPort here, just call bound_http_.host() and get Done PS11, Line 94: HostPort(bound_http_).port() > bound_http_.port() Done Line 95: Shutdown(); > Restart() has unusual semantics in the minicluster* classes. They require S Ah I see, makes sense. I'll save that for another patch then; I don't have anything against those semantics. To just keep the original behavior (with no Shutdown()), I'll just take out Shutdown() from Restart(). In all the tests, if Shutdown() is always called beforehand, the Shutdown() here would just be a no-op anyway. http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/master/mini_master.cc File src/kudu/master/mini_master.cc: Line 100: if (master_) { > warning: an integer is interpreted as a character code when assigning it to Done http://gerrit.cloudera.org:8080/#/c/7211/12/src/kudu/tserver/mini_tablet_server-test.cc File src/kudu/tserver/mini_tablet_server-test.cc: Line 21 > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/mini_tablet_server.cc File src/kudu/tserver/mini_tablet_server.cc: PS11, Line 65: "data$0 > I know this is subjective but I think this would be a little more readable Done http://gerrit.cloudera.org:8080/#/c/7211/11/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: Line 2486: TEST_F(TabletServerTest, TestMultiDirServer) { > This should probably be in its own file if this is just a unit test for the Done -- 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: 11 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
