

<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch and the fixes. New version LGTM. I couldn’t find the other patches in my mailbox though, could you please resend them? If you need my review, of course.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 25 августа 2020, 15:49 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15983597800092920506_BODY">Vlad, thanks for review!<br>Patches updated in a branch and please see my answers inline.<br><br>On 00:00 Tue 21 Jul , Vladislav Shpilevoy wrote:<br>> Thanks for the patch!<br>><br>> See 8 comments below.<br>><br>> On 09.07.2020 19:16, <a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a> wrote:<br>> > From: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br>> ><br>> > Part of #5055<br>> > Part of #4849<br>> > ---<br>> > test/replication/qsync_basic.result | 85 +++++++++++++++++++++++++++<br>> > test/replication/qsync_basic.test.lua | 31 ++++++++++<br>> > 2 files changed, 116 insertions(+)<br>> ><br>> > diff --git a/test/replication/qsync_basic.result b/test/replication/qsync_basic.result<br>> > index ab4be0c7e..464df75a7 100644<br>> > --- a/test/replication/qsync_basic.result<br>> > +++ b/test/replication/qsync_basic.result<br>> > @@ -32,6 +32,14 @@ s2.is_sync<br>> > | - false<br>> > | ...<br>> ><br>> > +--<br>> > +-- gh-4849: clear synchro queue with unconfigured box<br>> > +--<br>> > +box.ctl.clear_synchro_queue()<br>><br>> 1. Enough to test that it does not crash here. No need to<br>> check result.<br><br>Well, removed assignment to variable.<br><br>> > + | ---<br>> > + | - -1<br>> > + | ...<br>> > +<br>> > -- Net.box takes sync into account.<br>> > box.schema.user.grant('guest', 'super')<br>> > | ---<br>> > @@ -553,6 +561,82 @@ box.space.sync:select{7}<br>> > | - - [7]<br>> > | ...<br>> ><br>> > +--<br>> > +-- gh-4849: clear synchro queue on a replica<br>> > +--<br>> > +test_run:switch('default')<br>> > + | ---<br>> > + | - true<br>> > + | ...<br>> > +box.cfg{replication_synchro_quorum = 3, replication_synchro_timeout = 2}<br>><br>> 2. If you want this timeout to fail, it is too big. The test will be too<br>> long. If it is supposed not to fail, then it is way too small. If you want<br>> it to fail, it should be something like 0.001. If you want it to hold, it<br>> should be 1000 to be sure. Keep in mind that you can change the timeout on<br>> fly. That allows to build quite complex reactive tests completely event-based.<br><br>set replication_synchro_timeout value to 1000<br><br>> > + | ---<br>> > + | ...<br>> > +f1 = fiber.create(box.space.sync.replace, box.space.sync, {9})<br>><br>> 3. You need to extract the exact result value. Use pcall for that, and print<br>> its result after the fiber is dead. See other examples with fiber.create() in<br>> qsync test suite. The same for the next test case.<br><br>Done.<br><br>> > + | ---<br>> > + | ...<br>> > +f1:status()<br>> > + | ---<br>> > + | - suspended<br>> > + | ...<br>> > +test_run:switch('replica')<br>> > + | ---<br>> > + | - true<br>><br>> 4. Better wait until the value is delivered. Otherwise you can switch to<br>> replica before master finishes WAL write, and the queue will be empty here.<br>> Try<br>><br>> test_run:wait_cond(function() return box.space.sync:get{9} ~= nil end)<br><br>Agree, added wait_cond().<br><br>> > + | ...<br>> > +box.ctl.clear_synchro_queue()<br>> > + | ---<br>> > + | - 0<br>> > + | ...<br>> > +box.space.sync:select{9}<br>> > + | ---<br>> > + | - []<br>><br>> 5. If select returns 9 before queue clean, and returns empty afterwards,<br>> it means the queue was cleared. So here the queue size is not really needed,<br>> as you can see.<br><br>removed this and next select{} statements<br><br>> > + | ...<br>> > +test_run:switch('default')<br>> > + | ---<br>> > + | - true<br>> > + | ...<br>> > +box.space.sync:select{9}<br>> > + | ---<br>> > + | - []<br>> > + | ...<br>> > +f1:status()<br>> > + | ---<br>> > + | - dead<br>> > + | ...<br>> > +<br>> > +--<br>> > +-- gh-4849: clear synchro queue on a master<br>><br>> 6. Since the previous test the replica's WAL is different from master's.<br>> I am not sure the replication is still alive.<br><br>Test with clear_synchro_queue() on master keeps master alive at the end<br>of test so I moved it before the same test for the replica. Also added<br>note for others that cluster may be in a broken state here.<br><br>> > +--<br>> > +test_run:switch('default')<br>> > + | ---<br>> > + | - true<br>> > + | ...<br>> > +box.cfg{replication_synchro_quorum = 3, replication_synchro_timeout = 2}<br>> > + | ---<br>> > + | ...<br>> > +f1 = fiber.create(box.space.sync.replace, box.space.sync, {10})<br>> > + | ---<br>> > + | ...<br>> > +f1:status()<br>> > + | ---<br>> > + | - suspended<br>> > + | ...<br>> > +box.ctl.clear_synchro_queue()<br>> > + | ---<br>> > + | - -2<br>> > + | ...<br>> > +box.space.sync:select{10}<br>> > + | ---<br>> > + | - - [10]<br>><br>> 7. Why is it 10? The quorum is not reached, it should have been rolled back.<br><br>Added "box.cfg{replication_synchro_timeout = 0.1}" before clear_synchro_queue()<br>call and now tx is rolled back. Also I've added checks of current value with<br>wait_cond() that is more reliable than select{} on a master and replica.<br>Updated output looks like this:<br><br>box.ctl.clear_synchro_queue()<br> | ---<br> | ...<br>test_run:switch('replica')<br> | ---<br> | - true<br> | ...<br>test_run:wait_cond(function() return box.space.sync:get{10} == nil end)<br> | ---<br> | - true<br> | ...<br>test_run:switch('default')<br> | ---<br> | - true<br> | ...<br>test_run:wait_cond(function() return f:status() == 'dead' end)<br> | ---<br> | - true<br> | ...<br>ok, err<br> | ---<br> | - false<br> | - Quorum collection for a synchronous transaction is timed out<br> | ...<br>test_run:wait_cond(function() return box.space.sync:get{10} == nil end)<br> | ---<br> | - true<br> | ...<br><br>> > + | ...<br>> > +test_run:switch('replica')<br>> > + | ---<br>> > + | - true<br>> > + | ...<br>> > +box.space.sync:select{10}<br>> > + | ---<br>> > + | - - [10]<br>> > + | ...<br>> > +<br>> > -- Cleanup.<br>> > test_run:cmd('switch default')<br>> > | ---<br>> > @@ -576,6 +660,7 @@ test_run:cmd('delete server replica')<br>> > | ...<br>> > box.space.test:drop()<br>> > | ---<br>> > + | - error: A rollback for a synchronous transaction is received<br>><br>> 8. Why is it changed?<br><br>Perhaps it is because in previous version of test I haven't wait fibers 'dead'.<br>There is no error now:<br><br>box.space.test:drop()<br> | ---<br> | ...<br>box.space.sync:drop()<br> | ---<br> | ...<br><br>><br>> > | ...<br>> > box.space.sync:drop()<br>> > | ---</div></div></div></div></blockquote><div> </div><div>--<br>Serge Petrenko</div></BODY></HTML>