[kudu-CR] mini-cluster: support parallel multi-master clusters
Dan Burkert has removed Tidy Bot from this change. ( http://gerrit.cloudera.org:8080/8280 ) Change subject: mini-cluster: support parallel multi-master clusters .. Removed reviewer Tidy Bot. -- 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: deleteReviewer Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] mini-cluster: support parallel multi-master clusters
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8280 ) Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Reviewed-on: http://gerrit.cloudera.org:8080/8280 Reviewed-by: Adar Dembo Tested-by: Dan Burkert --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 249 insertions(+), 326 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Dan Burkert: Verified -- 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: merged Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 10 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] mini-cluster: support parallel multi-master clusters
Dan Burkert has removed a vote on this change. Change subject: mini-cluster: support parallel multi-master clusters .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 9: Verified+1 This has failed three times in a row on non-intersecting flakes, so I'm going to override. -- 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: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 03 May 2018 21:42:50 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 9: Code-Review+2 -- 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: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 03 May 2018 20:47:51 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#9). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 249 insertions(+), 326 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/9 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8280/8/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: PS8: > Could probably just revert the changes to this file. Done -- 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: 9 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 03 May 2018 19:21:40 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8280/8/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: PS8: Could probably just revert the changes to this file. -- 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: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 03 May 2018 19:18:12 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/integration-tests/master_migration-itest.cc@89
PS5, Line 89: // Collect and keep alive the set of master sockets bound with
SO_REUSEPORT.
> Can you add some comments here explaining why we're doing this?
Done
http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/master_migration-itest.cc@89
PS7, Line 89: SO_REUSEPOR
> SO_REUSEPORT
Done
http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/registration-test.cc
File src/kudu/integration-tests/registration-test.cc:
http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/registration-test.cc@272
PS7, Line 272: const int startup_rows_inserted =
GetCatalogMetric(METRIC_rows_inserted);
:
: // Add a table, make sure it reports itself.
> Hmm, the fact that you had to add this means the patch has changed some sem
Done
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/mini-cluster/internal_mini_cluster.cc
File src/kudu/mini-cluster/internal_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/mini-cluster/internal_mini_cluster.cc@149
PS5, Line 149: if (num_masters > 1) {
: mini_master->SetMasterAddresses(master_rpc_addrs);
: }
> Not sure I understand the new condition; if num_masters was 0, won't this l
Good catch, this was supposed to be > 1. The Internal mini cluster already had
the equivalent.
--
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: 8
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 03 May 2018 19:13:30 +
Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#8). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 28 files changed, 251 insertions(+), 327 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/8 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/master_migration-itest.cc File src/kudu/integration-tests/master_migration-itest.cc: http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/master_migration-itest.cc@89 PS7, Line 89: SO_REUSPORT SO_REUSEPORT http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: http://gerrit.cloudera.org:8080/#/c/8280/7/src/kudu/integration-tests/registration-test.cc@272 PS7, Line 272: ASSERT_OK(cluster_->mini_master() : ->master() : ->WaitUntilCatalogManagerIsLeaderAndReadyForTests(MonoDelta::FromSeconds(10))); Hmm, the fact that you had to add this means the patch has changed some semantics. My guess is that InternalMiniCluster::Start used to do this for single masters, and when you unified its single/master code paths, this got dropped. Could you restore it there? Alternatively, if you do want to make these semantics more consistent and clear, I would recommend taking this further. ExternalMiniCluster provides no guarantees about the state of the catalogs after starting a cluster, regardless of the number of masters. I think that's how InternalMiniCluster should behave too; any tests that need the catalog to be initialized, or for a leader master to be elected should wait for that explicitly. But I understand if you feel like that's out of scope for this patch, in which case my request is that you restore the old semantics as I wrote above. -- 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: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 03 May 2018 18:40:43 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#7). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 28 files changed, 253 insertions(+), 328 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/7 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#6). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 244 insertions(+), 326 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/6 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/integration-tests/master_migration-itest.cc
File src/kudu/integration-tests/master_migration-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/integration-tests/master_migration-itest.cc@89
PS5, Line 89: vector> reserved_sockets;
Can you add some comments here explaining why we're doing this?
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/mini-cluster/internal_mini_cluster.cc
File src/kudu/mini-cluster/internal_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/8280/5/src/kudu/mini-cluster/internal_mini_cluster.cc@149
PS5, Line 149: if (num_masters > 0) {
: mini_master->SetMasterAddresses(master_rpc_addrs);
: }
Not sure I understand the new condition; if num_masters was 0, won't this loop
be skipped?
--
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: 5
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 02 May 2018 23:59:47 +
Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 5: (2 comments) > Patch Set 5: > > > Build Failed > > > > http://jenkins.kudu.apache.org/job/kudu-gerrit/13260/ : FAILURE > > It seems the pattern for the expected error messages should be also updated > due to the change in util/net/socket.cc:528 > > Expected: contains regular expression "Connection reset by peer|failed to > read from TLS socket|got EOF from remote|Protocol wrong type for > socket|sendmsg error: Invalid argument" > Actual: "Network error: recv got EOF from 127.0.0.1:50990 (error 108)" http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc@317 PS3, Line 317: > Could you add that in as a TODO? Done http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc@119 PS3, Line 119: > Add the TODO here too. Done -- 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: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 02 May 2018 19:27:23 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#5). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 242 insertions(+), 326 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/5 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc@317 PS3, Line 317: // Collect and keep alive the set of master sockets bound with SO_REUSPORT > I've given up on that goal, for this patch. If we want to explore that in Could you add that in as a TODO? http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc@119 PS3, Line 119: > Done Add the TODO here too. -- 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: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 01 May 2018 20:26:12 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
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: (4 comments) http://gerrit.cloudera.org:8080/#/c/8280/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8280/3//COMMIT_MSG@11 PS3, Line 11: This allows for a few : simplifications: > There's only one simplification in this list now. Done http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc@317 PS3, Line 317: vector> reserved_sockets; > Worth a comment explaining why it's OK for this to go out of scope after al I've given up on that goal, for this patch. If we want to explore that in subsequent patches this will be a good base to build on. As a result I've removed the change to the default IP subnet config for linx. http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc@119 PS3, Line 119: vector> reserved_sockets; > Likewise. Done http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/mini_cluster.cc@80 PS3, Line 80: socket->reset(new Socket()); > Style nit: prefer the calling convention where OUT params are unmodified in Done -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 01 May 2018 19:45:54 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#4). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. As a result, master addresses for multi-master mini clusters no longer need to hard-coded master ports, and the associated ctest resource lock. This also significantly lessens the chances that port conflicts will occur, although it is still possible when masters are restarted during tests. To mitigate that on Linux, tests are still run in a unique subnet. WIP: This commit removes the ability to set the desired master ports. master_migration-itest relies on this functionality, so it's currently broken. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 180 insertions(+), 276 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/4 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/8280/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8280/3//COMMIT_MSG@11 PS3, Line 11: This allows for a few : simplifications: There's only one simplification in this list now. http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/external_mini_cluster.cc@317 PS3, Line 317: vector> reserved_sockets; Worth a comment explaining why it's OK for this to go out of scope after all of the masters have been started. Actually, if we're going to close the different-process-grabs-our-port-during-restart race, shouldn't we keep this alive for the lifetime of the cluster? http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/internal_mini_cluster.cc@119 PS3, Line 119: vector> reserved_sockets; Likewise. http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8280/3/src/kudu/mini-cluster/mini_cluster.cc@80 PS3, Line 80: socket->reset(new Socket()); Style nit: prefer the calling convention where OUT params are unmodified in the event of an error. So, set up a local unique_ptr and swap to 'socket' when you can no longer fail. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Apr 2018 18:27:07 + Gerrit-HasComments: Yes
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 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 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.
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#3). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. This allows for a few simplifications: * master addresses in multi-master clusters can be reserved up-front, which removes any chance of a race condition in binding. As a result, multi-master clusters no longer need hard-coded ports, and tests using multi-master clusters no longer need to run in sequence. WIP: This commit removes the ability to set the desired master ports. master_migration-itest relies on this functionality, so it's currently broken. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-auth-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 27 files changed, 170 insertions(+), 276 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/3 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 2: It's coming back to me now; at least one of the issues is that we rely on lsof to tell us if ports are bound / a service is ready, and the reuse_port trick messes with that. -- 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: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Oct 2017 18:55:03 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
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 2: I got some time to bang on this on a recent flight, but I still don't think it's in a comittable state. I recall running into an issue with reuse port, but I don't remember what (I recall thinking I'd need a linux test box, and had no internet access). Anyway, I'll pick this back up when I have more time -- 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: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Oct 2017 18:47:52 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#2). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. This allows for a few simplifications: * master addresses in multi-master clusters can be reserved up-front, which removes any chance of a race condition in binding. As a result, multi-master clusters no longer need hard-coded ports. As a result, tests using multi-master clusters no longer need to run in sequence. * external mini cluster tablet servers also now use a reserved port, so that when the tablet server processes is restarted there is no chance of a port conflict. As a result, we no longer are required to use the 'unique loopback' hack on Linux. Internal mini cluster tablet servers still bind to a non-reserved port, since we never restart internal mini-cluster tablet servers. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 25 files changed, 162 insertions(+), 236 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/2 -- 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: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] mini-cluster: support parallel multi-master clusters
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)
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 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
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8280 to review the following change. Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters The external mini cluster previously required hard-coded ports for multi-master clusters, which in practice meant that tests which require multi-master clusters must be run serially. This commit changes the external mini cluster to assign ports so that many multi-master clusters can coexist in a single port-space at once. To do this, the mini-cluster coordinator binds to N sockets with the SO_REUSEPORT socket options, which effectively reserves the ports. Next, it uses these reserved ports to start the masters. This method is also advantageous in that in keeps the ports reserved while the masters may be temporarily shutdown and restarted during the life of the mini-cluster. WIP: This commit removes the ability to set the desired master ports. master_migration-itest relies on this functionality, so it's currently broken. I'm not sure whether it would be better to restore this functionality, or rewrite the test so that it doesn't need it. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 23 files changed, 125 insertions(+), 204 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/1 -- 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: newchange Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo
