From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0C1DB469719 for ; Tue, 8 Sep 2020 10:44:41 +0300 (MSK) Date: Tue, 8 Sep 2020 10:44:38 +0300 From: Mergen Imeev Message-ID: <20200908074438.GA3944@tarantool.org> References: <20200907144630.GA194562@tarantool.org> <20200907181149.GA25550@hpalx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200907181149.GA25550@hpalx> Subject: Re: [Tarantool-patches] [PATCH v2] Divide replication/misc.test.lua List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes. I have two comments, but both are not very important. Other that that, LGTM. On Mon, Sep 07, 2020 at 09:11:49PM +0300, Alexander V. Tikhonov wrote: > Hi Mergen, thanks for the review, please check my comments below on your > comments and diff of the changes. > > On Mon, Sep 07, 2020 at 05:46:30PM +0300, Mergen Imeev wrote: > > Hi! Thank you for the patch. See 4 comments below. > > > > On Fri, Sep 04, 2020 at 09:13:58PM +0300, Alexander V. Tikhonov wrote: > > > To fix flaky issues of replication/misc.test.lua the test had to be > > > divided into smaller tests to be able to localize the flaky results: > > > > > > gh-2991-misc-assert-on-server-die.test.lua > > > gh-3111-misc-rebootstrap-from-ro-master.test.lua > > > gh-3160-misc-heartbeats-on-master-changes.test.lua > > > gh-3247-misc-value-not-replicated-on-iproto-request.test.lua > > > gh-3510-misc-assert-replica-on-applier-disconnect.test.lua > > > gh-3606-misc-crash-on-box-concurrent-update.test.lua > > > gh-3610-misc-assert-connecting-master-twice.test.lua > > > gh-3637-misc-no-panic-on-connected.test.lua > > > gh-3642-misc-no-socket-leak-on-replica-disconnect.test.lua > > > gh-3704-misc-replica-checks-cluster-id.test.lua > > > gh-3711-misc-no-restart-on-same-configuration.test.lua > > > gh-3760-misc-return-on-quorum-0.test.lua > > > gh-4399-misc-no-failure-on-error-reading-wal.test.lua > > > gh-4424-misc-orphan-on-reconfiguration-error.test.lua 1. I think the names are too long, but there is no rule governing that, so I guess that's okay. In addition, some names do not reflect the content of the test, for example gh-2991, gh-3247, gh-3637. > > 1. I don't think 'misc' is needed in test names. > > I don't think so. The test name must consists the name of the initial > test to be able to check the history of the changes. > 2. I have not seen such a rule anywhere. In that case, wouldn't it be better to leave a comment inside? > > > diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua > > > new file mode 100644 > > > index 000000000..30089ac23 > > > --- /dev/null > > > +++ b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua > > > @@ -0,0 +1,14 @@ > > > +-- > > > +-- gh-3760: replication quorum 0 on reconfiguration should return > > > +-- from box.cfg immediately. > > > +-- > > > +replication = box.cfg.replication > > > +box.cfg{ \ > > > + replication = {}, \ > > > + replication_connect_quorum = 0, \ > > > + replication_connect_timeout = 1000000 \ > > > +} > > > +-- The call below would hang, if quorum 0 is ignored, or checked > > > +-- too late. > > > +box.cfg{replication = {'localhost:12345'}} > > > +box.info.status > > 2. I think there is something wrong with this test. I mean, you change the > > box.cfg options, but you don't restore them. > > Ok, as we discussed I've returned back the code removed after the first > review. > > > > diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua > > > new file mode 100644 > > > index 000000000..a926ae590 > > > --- /dev/null > > > +++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua > > > @@ -0,0 +1,38 @@ > > > +test_run = require('test_run').new() > > > +test_run:cmd("restart server default") > > > +fiber = require('fiber') > > 3. This is not needed here. > > Ok, sure, removed. > > > > --- a/test/replication/suite.cfg > > > +++ b/test/replication/suite.cfg > > > @@ -1,6 +1,19 @@ > > > { > > > "anon.test.lua": {}, > > > - "misc.test.lua": {}, > > > + "misc_assert_connecting_master_twice_gh-3610.test.lua": {}, > > > + "misc_assert_on_server_die_gh-2991.test.lua": {}, > > > + "misc_assert_replica_on_applier_disconnect_gh-3510.test.lua": {}, > > > + "misc_crash_on_box_concurrent_update_gh-3606.test.lua": {}, > > > + "misc_heartbeats_on_master_changes_gh-3160.test.lua": {}, > > > + "misc_no_failure_on_error_reading_wal_gh-4399.test.lua": {}, > > > + "misc_no_panic_on_connected_gh-3637.test.lua": {}, > > > + "misc_no_restart_on_same_configuration_gh-3711.test.lua": {}, > > > + "misc_no_socket_leak_on_replica_disconnect_gh-3642.test.lua": {}, > > > + "misc_orphan_on_reconfiguration_error_gh-4424.test.lua": {}, > > > + "misc_rebootstrap_from_ro_master_gh-3111.test.lua": {}, > > > + "misc_replica_checks_cluster_id_gh-3704.test.lua": {}, > > > + "misc_return_on_quorum_0_gh-3760.test.lua": {}, > > > + "misc_value_not_replicated_on_iproto_request_gh-3247.test.lua": {}, > > 4. Wrong names of the tests. > > Right, corrected. > > Check the changes bellow: > > diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.result b/test/replication/gh-3760-misc-return-on-quorum-0.result > index 79295f5c2..ed8ae529b 100644 > --- a/test/replication/gh-3760-misc-return-on-quorum-0.result > +++ b/test/replication/gh-3760-misc-return-on-quorum-0.result > @@ -1,3 +1,12 @@ > +replication_connect_timeout = box.cfg.replication_connect_timeout > +--- > +... > +replication_connect_quorum = box.cfg.replication_connect_quorum > +--- > +... > +box.cfg{replication="12345", replication_connect_timeout=0.1, replication_connect_quorum=1} > +--- > +... > -- > -- gh-3760: replication quorum 0 on reconfiguration should return > -- from box.cfg immediately. > @@ -21,3 +30,10 @@ box.info.status > --- > - running > ... > +box.cfg{ \ > + replication = {}, \ > + replication_connect_quorum = replication_connect_quorum, \ > + replication_connect_timeout = replication_connect_timeout \ > +} > +--- > +... > diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua > index 30089ac23..dc8508004 100644 > --- a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua > +++ b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua > @@ -1,3 +1,7 @@ > +replication_connect_timeout = box.cfg.replication_connect_timeout > +replication_connect_quorum = box.cfg.replication_connect_quorum > +box.cfg{replication="12345", replication_connect_timeout=0.1, replication_connect_quorum=1} > + > -- > -- gh-3760: replication quorum 0 on reconfiguration should return > -- from box.cfg immediately. > @@ -12,3 +16,8 @@ box.cfg{ \ > -- too late. > box.cfg{replication = {'localhost:12345'}} > box.info.status > +box.cfg{ \ > + replication = {}, \ > + replication_connect_quorum = replication_connect_quorum, \ > + replication_connect_timeout = replication_connect_timeout \ > +} > diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result > index 46b4f6464..a105e54b3 100644 > --- a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result > +++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result > @@ -2,9 +2,6 @@ test_run = require('test_run').new() > --- > ... > test_run:cmd("restart server default") > -fiber = require('fiber') > ---- > -... > -- > -- gh-4399 Check that an error reading WAL directory on subscribe > -- doesn't lead to a permanent replication failure. > diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua > index a926ae590..f071d97c1 100644 > --- a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua > +++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua > @@ -1,6 +1,5 @@ > test_run = require('test_run').new() > test_run:cmd("restart server default") > -fiber = require('fiber') > > -- > -- gh-4399 Check that an error reading WAL directory on subscribe > diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg > index e21daa5ad..f9f7055b7 100644 > --- a/test/replication/suite.cfg > +++ b/test/replication/suite.cfg > @@ -1,19 +1,19 @@ > { > "anon.test.lua": {}, > - "misc_assert_connecting_master_twice_gh-3610.test.lua": {}, > - "misc_assert_on_server_die_gh-2991.test.lua": {}, > - "misc_assert_replica_on_applier_disconnect_gh-3510.test.lua": {}, > - "misc_crash_on_box_concurrent_update_gh-3606.test.lua": {}, > - "misc_heartbeats_on_master_changes_gh-3160.test.lua": {}, > - "misc_no_failure_on_error_reading_wal_gh-4399.test.lua": {}, > - "misc_no_panic_on_connected_gh-3637.test.lua": {}, > - "misc_no_restart_on_same_configuration_gh-3711.test.lua": {}, > - "misc_no_socket_leak_on_replica_disconnect_gh-3642.test.lua": {}, > - "misc_orphan_on_reconfiguration_error_gh-4424.test.lua": {}, > - "misc_rebootstrap_from_ro_master_gh-3111.test.lua": {}, > - "misc_replica_checks_cluster_id_gh-3704.test.lua": {}, > - "misc_return_on_quorum_0_gh-3760.test.lua": {}, > - "misc_value_not_replicated_on_iproto_request_gh-3247.test.lua": {}, > + "gh-3610-misc-assert-connecting-master-twice.test.lua": {}, > + "gh-2991-misc-assert-on-server-die.test.lua": {}, > + "gh-3510-misc-assert-replica-on-applier-disconnect.test.lua": {}, > + "gh-3606-misc-crash-on-box-concurrent-update.test.lua": {}, > + "gh-3160-misc-heartbeats-on-master-changes.test.lua": {}, > + "gh-4399-misc-no-failure-on-error-reading-wal.test.lua": {}, > + "gh-3637-misc-no-panic-on-connected.test.lua": {}, > + "gh-3711-misc-no-restart-on-same-configuration.test.lua": {}, > + "gh-3642-misc-no-socket-leak-on-replica-disconnect.test.lua": {}, > + "gh-4424-misc-orphan-on-reconfiguration-error.test.lua": {}, > + "gh-3111-misc-rebootstrap-from-ro-master.test.lua": {}, > + "gh-3704-misc-replica-checks-cluster-id.test.lua": {}, > + "gh-3760-misc-return-on-quorum-0.test.lua": {}, > + "gh-3247-misc-value-not-replicated-on-iproto-request.test.lua": {}, > "once.test.lua": {}, > "on_replace.test.lua": {}, > "status.test.lua": {}, >