Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
@ 2021-12-08 10:44 Serge Petrenko via Tarantool-patches
  2021-12-08 11:00 ` Vladimir Davydov via Tarantool-patches
  2021-12-08 23:45 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-12-08 10:44 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

The test fails quite often with one of the following results.

Either

[001] @@ -60,7 +60,7 @@
[001]  ...
[001]  test_timeout()
[001]  ---
[001] -- true
[001] +- false
[001]  ...
[001]  test_run:cmd("switch default")
[001]  ---
[001]

Or

[034]  test_timeout()
[034]  ---
[034] -- true
[034] +- error: 'replicas are not in the follow status'
[034]  ...

Both errors are caused by wait_cond checking saved
`box.info.replication` values instead of actual `box.info.replication`
output. Fix this.

Also, the test's quite long for no reason.

The wait_cond waiting for replication to break lasts a whole
`replication_timeout`, which is excess.

The test's idea, as stated in the commit that has introduced it
(195d4462: Send relay heartbeat if wal changes won't be send) is to
constantly relay some data to the remote peers and make
it so their relays never send heartbeats.

If we wait for a whole replication timeout between inserts, there's a high
chance of peer relays waking up naturally (after replication_timeout passes).

In this case the test tests nothing. Fix the issue by waiting for ~ 1/5
of replication_timeout in between the despatches. This also reduces test
run time from ~4.5 to ~1.5 seconds on my machine.

Remove the test from fragile list, since it shouldn't be flaky anymore.

Closes #4940
---
https://github.com/tarantool/tarantool/issues/4940
https://github.com/tarantool/tarantool/tree/sp/gh-3160-fix

 ...0-misc-heartbeats-on-master-changes.result | 22 ++++++++++-------
 ...misc-heartbeats-on-master-changes.test.lua | 24 ++++++++++++-------
 test/replication/suite.ini                    |  4 ----
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/test/replication/gh-3160-misc-heartbeats-on-master-changes.result b/test/replication/gh-3160-misc-heartbeats-on-master-changes.result
index 86e5ddfa0..101317ebd 100644
--- a/test/replication/gh-3160-misc-heartbeats-on-master-changes.result
+++ b/test/replication/gh-3160-misc-heartbeats-on-master-changes.result
@@ -26,23 +26,27 @@ test_run:cmd("setopt delimiter ';'")
 ---
 - true
 ...
-function wait_not_follow(replicaA, replicaB)
+local function replica(id)
+    return box.info.replication[id].upstream
+end
+
+function wait_not_follow(id_a, id_b)
     return test_run:wait_cond(function()
-        return replicaA.status ~= 'follow' or replicaB.status ~= 'follow'
-    end, box.cfg.replication_timeout)
+        return replica(id_a).status ~= 'follow' or
+               replica(id_b).status ~= 'follow'
+    end, box.cfg.replication_timeout / 5)
 end;
 ---
 ...
 function test_timeout()
-    local replicaA = box.info.replication[1].upstream or box.info.replication[2].upstream
-    local replicaB = box.info.replication[3].upstream or box.info.replication[2].upstream
-    local follows = test_run:wait_cond(function()
-        return replicaA.status == 'follow' or replicaB.status == 'follow'
-    end)
+    local id_a = box.info.id % 3 + 1
+    local id_b = id_a % 3 + 1
+    local follows = test_run:wait_upstream(id_a, {status = 'follow'})
+    follows = follows and test_run:wait_upstream(id_b, {status = 'follow'})
     if not follows then error('replicas are not in the follow status') end
     for i = 0, 99 do
         box.space.test_timeout:replace({1})
-        if wait_not_follow(replicaA, replicaB) then
+        if wait_not_follow(id_a, id_b) then
             require('log').error(box.info.replication)
             return false
         end
diff --git a/test/replication/gh-3160-misc-heartbeats-on-master-changes.test.lua b/test/replication/gh-3160-misc-heartbeats-on-master-changes.test.lua
index bfc9f854f..21a6f1443 100644
--- a/test/replication/gh-3160-misc-heartbeats-on-master-changes.test.lua
+++ b/test/replication/gh-3160-misc-heartbeats-on-master-changes.test.lua
@@ -10,21 +10,27 @@ test_run:cmd("switch autobootstrap3")
 test_run = require('test_run').new()
 _ = box.schema.space.create('test_timeout'):create_index('pk')
 test_run:cmd("setopt delimiter ';'")
-function wait_not_follow(replicaA, replicaB)
+
+local function replica(id)
+    return box.info.replication[id].upstream
+end
+
+function wait_not_follow(id_a, id_b)
     return test_run:wait_cond(function()
-        return replicaA.status ~= 'follow' or replicaB.status ~= 'follow'
-    end, box.cfg.replication_timeout)
+        return replica(id_a).status ~= 'follow' or
+               replica(id_b).status ~= 'follow'
+    end, box.cfg.replication_timeout / 5)
 end;
+
 function test_timeout()
-    local replicaA = box.info.replication[1].upstream or box.info.replication[2].upstream
-    local replicaB = box.info.replication[3].upstream or box.info.replication[2].upstream
-    local follows = test_run:wait_cond(function()
-        return replicaA.status == 'follow' or replicaB.status == 'follow'
-    end)
+    local id_a = box.info.id % 3 + 1
+    local id_b = id_a % 3 + 1
+    local follows = test_run:wait_upstream(id_a, {status = 'follow'})
+    follows = follows and test_run:wait_upstream(id_b, {status = 'follow'})
     if not follows then error('replicas are not in the follow status') end
     for i = 0, 99 do
         box.space.test_timeout:replace({1})
-        if wait_not_follow(replicaA, replicaB) then
+        if wait_not_follow(id_a, id_b) then
             require('log').error(box.info.replication)
             return false
         end
diff --git a/test/replication/suite.ini b/test/replication/suite.ini
index e20d63a6e..347710f58 100644
--- a/test/replication/suite.ini
+++ b/test/replication/suite.ini
@@ -21,10 +21,6 @@ fragile = {
             "issues": [ "gh-4351" ],
             "checksums": [ "acd88b48b0046ec52346274eeeef0b25", "a645ff7616b5caf0fcd2099022b776bf", "eb3e92564ba71e7b7c458050223f4d57" ]
         },
-        "gh-3160-misc-heartbeats-on-master-changes.test.lua": {
-            "issues": [ "gh-4940" ],
-            "checksums": [ "945521821b8199c59716e969d89d953d", "b4e60f8ec2d4340bc0324f73e2cc8a01", "c7054aec18a7a983c717f1b92dd1434c", "09500c4d118ace1e05b23665ba055bf5", "60d4cbd20d4c646deb9464f82fabffb4" ]
-        },
         "skip_conflict_row.test.lua": {
             "issues": [ "gh-4958" ],
             "checksums": [ "a21f07339237cd9d0b8c74e144284449", "0359b0b1cc80052faf96972959513694", "ef104dfd04afa7c75087de13246e3eb0" ]
-- 
2.30.1 (Apple Git-130)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 10:44 [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test Serge Petrenko via Tarantool-patches
@ 2021-12-08 11:00 ` Vladimir Davydov via Tarantool-patches
  2021-12-08 11:13   ` Serge Petrenko via Tarantool-patches
  2021-12-08 23:45 ` Vladislav Shpilevoy via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Vladimir Davydov via Tarantool-patches @ 2021-12-08 11:00 UTC (permalink / raw)
  To: Serge Petrenko via Tarantool-patches

Merged into master, 2.8, 1.10.

Please don't send emails. PR is enough :-)

Thanks!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 11:00 ` Vladimir Davydov via Tarantool-patches
@ 2021-12-08 11:13   ` Serge Petrenko via Tarantool-patches
  2021-12-08 11:25     ` Vladimir Davydov via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-12-08 11:13 UTC (permalink / raw)
  To: Vladimir Davydov, Serge Petrenko via Tarantool-patches



08.12.2021 14:00, Vladimir Davydov via Tarantool-patches пишет:
> Merged into master, 2.8, 1.10.
>
> Please don't send emails. PR is enough :-)

Some people prefer emails over PRs.

I think everyone should review the patches where it suits
them best, so I open PRs for you and send emails to Vlad.
(even though this is double the hustle).

Thanks for the review!
>
> Thanks!

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 11:13   ` Serge Petrenko via Tarantool-patches
@ 2021-12-08 11:25     ` Vladimir Davydov via Tarantool-patches
  2021-12-08 11:56       ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Davydov via Tarantool-patches @ 2021-12-08 11:25 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: Serge Petrenko via Tarantool-patches

On Wed, Dec 08, 2021 at 02:13:04PM +0300, Serge Petrenko wrote:
> 
> 
> 08.12.2021 14:00, Vladimir Davydov via Tarantool-patches пишет:
> > Merged into master, 2.8, 1.10.
> > 
> > Please don't send emails. PR is enough :-)
> 
> Some people prefer emails over PRs.
> 
> I think everyone should review the patches where it suits
> them best, so I open PRs for you and send emails to Vlad.
> (even though this is double the hustle).

This is confusing. I noticed the email only after I merged the PR.
In the PR, I was the only reviewer so I reckoned it's okay to merge,
but it turned out the patch was also sent to Vlad via email...

Also, when the same patch is reviewed both via email and PR, the same
comment might be raised twice by different reviewers or reviewers may
suggest different things being not aware of each other's arguments and
you'll have to reconcile.

Vlad can review patches in PRs AFAICS - he reviews my patches in PRs,
for instance.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 11:25     ` Vladimir Davydov via Tarantool-patches
@ 2021-12-08 11:56       ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-12-08 11:56 UTC (permalink / raw)
  To: Vladimir Davydov; +Cc: Serge Petrenko via Tarantool-patches



08.12.2021 14:25, Vladimir Davydov пишет:
> On Wed, Dec 08, 2021 at 02:13:04PM +0300, Serge Petrenko wrote:
>>
>> 08.12.2021 14:00, Vladimir Davydov via Tarantool-patches пишет:
>>> Merged into master, 2.8, 1.10.
>>>
>>> Please don't send emails. PR is enough :-)
>> Some people prefer emails over PRs.
>>
>> I think everyone should review the patches where it suits
>> them best, so I open PRs for you and send emails to Vlad.
>> (even though this is double the hustle).
> This is confusing. I noticed the email only after I merged the PR.
> In the PR, I was the only reviewer so I reckoned it's okay to merge,
> but it turned out the patch was also sent to Vlad via email...

Yes, that's my bad. I could've put Vlad into reviewers list as well.

>
> Also, when the same patch is reviewed both via email and PR, the same
> comment might be raised twice by different reviewers or reviewers may
> suggest different things being not aware of each other's arguments and
> you'll have to reconcile.

I agree. Though this particular patch was rather small, so I don't think it
could raise any disputes.

>
> Vlad can review patches in PRs AFAICS - he reviews my patches in PRs,
> for instance.

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 10:44 [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test Serge Petrenko via Tarantool-patches
  2021-12-08 11:00 ` Vladimir Davydov via Tarantool-patches
@ 2021-12-08 23:45 ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-09 10:19   ` Serge Petrenko via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-08 23:45 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: tarantool-patches

Hi! Thanks for the patch!

LGTM. Even though it is already pushed.

As for the email vs PR. I am fine with reviewing PRs if the emails make it
harder to Vova. Although I still prefer receiving emails in case I am the
only reviewer. Or all the other reviewers prefer emails too.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test
  2021-12-08 23:45 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-09 10:19   ` Serge Petrenko via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-12-09 10:19 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches



09.12.2021 02:45, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> LGTM. Even though it is already pushed.
>
> As for the email vs PR. I am fine with reviewing PRs if the emails make it
> harder to Vova. Although I still prefer receiving emails in case I am the
> only reviewer. Or all the other reviewers prefer emails too.
Ok, I see. Thanks!

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-12-09 10:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08 10:44 [Tarantool-patches] [PATCH] replication: fix flaky gh-3160-misc... test Serge Petrenko via Tarantool-patches
2021-12-08 11:00 ` Vladimir Davydov via Tarantool-patches
2021-12-08 11:13   ` Serge Petrenko via Tarantool-patches
2021-12-08 11:25     ` Vladimir Davydov via Tarantool-patches
2021-12-08 11:56       ` Serge Petrenko via Tarantool-patches
2021-12-08 23:45 ` Vladislav Shpilevoy via Tarantool-patches
2021-12-09 10:19   ` Serge Petrenko via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox