Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8280 )
Change subject: mini-cluster: support parallel multi-master clusters ...................................................................... Patch Set 1: (14 comments) Very nice. As we discussed in person, if this obviates the need for the UNIQUE_LOOPBACK setting, we should consider changing the default back to LOOPBACK (and plumbing UNIQUE_LOOPBACK through as a RunMiniCluster option). 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: The external mini cluster previously required hard-coded ports for This change is also to the internal mini cluster though, right? http://gerrit.cloudera.org:8080/#/c/8280/1//COMMIT_MSG@25 PS1, Line 25: rewrite the test so that it doesn't need it. I think this is the better path forward, since there's no real use case for master_rpc_ports except for this test. AFAICT, the issue is that in order to migrate from 1 to 3 masters, master_migration-itest needs to know the addresses of the two new masters, even though we've never started them. A possible solution is to extend ExternalMiniClusterOptions so that the caller can provide its own pre-reserved SO_REUSEPORT sockets (i.e. unique_ptr<Socket>) for some masters. If not provided, ExternalMiniCluster::Start() will create its own. Then, the test can create the sockets early and use them to set up the rewrite_raft_config CLI call, and pass them into the ExternalMiniCluster after. The catch is that the test currently uses one cluster instance that's created as part of the test fixture; that will have to change. 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 don't need it. 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: int num_masters_; Not used, can be removed. 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: int num_masters_; Can be removed. 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: int num_masters_; Can be removed; iterate on opts_.num_masters_ instead. 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: int num_masters_; Remove; use opts_.num_masters_ instead. http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/integration-tests/token_signer-itest.cc@127 PS1, Line 127: const int num_tablet_servers_ = 3; Not sure what purpose this serves either; could just as easily use opts_.num_tablet_servers_. 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: std::vector<uint16_t> master_rpc_ports() const override { What purpose does this now serve? http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/external_mini_cluster.h@345 PS1, Line 345: // Set of sockets which have been reserved for use by child daemons. These : // sockets are bound with SO_REUSEPORT so that the child daemon can re-use : // them. Only for masters though, right? Might want to clarify that. 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: Status ExternalMiniCluster::StartMasters() { Good cleanup here, but it might be better to pull it out into its own patch so it's easier to review just the socket pieces. 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: // Set of sockets which have been reserved for use by child daemons. These : // sockets are bound with SO_REUSEPORT so that the child daemon can re-use : // them. Masters only? 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: unique_ptr<Socket> socket(new Socket()); This is repeated in ExternalMiniCluster; consolidate into a helper? Could be a static Socket factory method, or something in socket_util.cc (doesn't exist yet). http://gerrit.cloudera.org:8080/#/c/8280/1/src/kudu/mini-cluster/internal_mini_cluster.cc@223 PS1, Line 223: vector<uint16_t> InternalMiniCluster::master_rpc_ports() const { Why is this implementation any different than ExternalMiniCluster's implementation? -- 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: 1 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 17 Oct 2017 00:06:30 +0000 Gerrit-HasComments: Yes
