Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8280 )
Change subject: mini-cluster: support parallel multi-master clusters ...................................................................... Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/8280/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8280/1//COMMIT_MSG@9 PS1, Line 9: This commit refactors the mini-clusters to internally use reserved > This change is also to the internal mini cluster though, right? Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/CMakeLists.txt@a63 PS1, Line 63: > RUN_SERIAL was just to reduce concurrent test load, IIRC, but we probably d Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_cert_authority-itest.cc File src/kudu/integration-tests/master_cert_authority-itest.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_cert_authority-itest.cc@186 PS1, Line 186: MasterServiceProxy proxy(messenger_, m->bound_rpc_addr(), m->bound_rpc_addr().host()); > Not used, can be removed. Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_failover-itest.cc File src/kudu/integration-tests/master_failover-itest.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_failover-itest.cc@150 PS1, Line 150: } > Can be removed. Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_replication-itest.cc File src/kudu/integration-tests/master_replication-itest.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/master_replication-itest.cc@135 PS1, Line 135: .set_range_partition_columns({ "key" }) > Can be removed; iterate on opts_.num_masters_ instead. Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/token_signer-itest.cc File src/kudu/integration-tests/token_signer-itest.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/token_signer-itest.cc@126 PS1, Line 126: InternalMiniClusterOptions opts_; > Remove; use opts_.num_masters_ instead. Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/token_signer-itest.cc@127 PS1, Line 127: unique_ptr<InternalMiniCluster> cluster_; > Not sure what purpose this serves either; could just as easily use opts_.nu Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.h@251 PS1, Line 251: int num_tablet_servers() const override { > What purpose does this now serve? Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.h@345 PS1, Line 345: : private: : FRIEND_T > Only for masters though, right? Might want to clarify that. Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.cc@295 PS1, Line 295: return paths; > Good cleanup here, but it might be better to pull it out into its own patch The diff between the old StartDistributedMasters and the new StartMasters is pretty minimal, pretty much only the reuse port stuff. http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.cc@318 PS1, Line 318: vector<HostPort> master_rpc_addrs; > warning: use emplace_back instead of push_back [modernize-use-emplace] Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.h File src/kudu/mini-cluster/internal_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.h@211 PS1, Line 211: : Env* const env_; : > Masters only? Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.cc@125 PS1, Line 125: RETURN_NOT_OK_PREPEND(ReserveDaemonSocket(MiniCluster::MASTER, i, opts_.bind_mode, > This is repeated in ExternalMiniCluster; consolidate into a helper? Could b Done http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.cc@223 PS1, Line 223: MiniTabletServer* InternalMiniCluster::mini_tablet_server_by_uuid(const string& uuid) const { > Why is this implementation any different than ExternalMiniCluster's impleme This has changed slightly, but the two implementations are slightly different. -- To view, visit http://gerrit.cloudera.org:8080/8280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Apr 2018 17:11:57 +0000 Gerrit-HasComments: Yes
