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 2A2354765E0 for ; Wed, 23 Dec 2020 17:14:18 +0300 (MSK) References: <20201222111408.48368-1-gorcunov@gmail.com> <8b570db3-858d-4475-1f9f-7e5c84321129@tarantool.org> <20201222171352.GC8261@grain> From: Vladislav Shpilevoy Message-ID: Date: Wed, 23 Dec 2020 15:14:15 +0100 MIME-Version: 1.0 In-Reply-To: <20201222171352.GC8261@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v6 0/5] qsync: evaluate replication_synchro_quorum dynamically List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml Hi! Thanks for the fixes! > diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result > new file mode 100644 > index 000000000..832c3f6e5 > --- /dev/null > +++ b/test/replication/gh-5446-qsync-eval-quorum.result > @@ -0,0 +1,298 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +engine = test_run:get_cfg('engine') > + | --- > + | ... > + > +box.schema.user.grant('guest', 'replication') > + | --- > + | ... > + > +-- Test syntax error > +box.cfg{replication_synchro_quorum = "aaa"} > + | --- > + | - error: 'Incorrect value for option ''replication_synchro_quorum'': [string "local > + | expr = [[aaa]]..."]:7: Expression should return a number' > + | ... > + > +-- Test out of bounds values > +box.cfg{replication_synchro_quorum = "N+1"} > + | --- > + | - error: 'Incorrect value for option ''replication_synchro_quorum'': the formula is > + | evaluated to the quorum 32 for replica number 31, which is out of range [1;31]' > + | ... > +box.cfg{replication_synchro_quorum = "N-1"} > + | --- > + | - error: 'Incorrect value for option ''replication_synchro_quorum'': the formula is > + | evaluated to the quorum 0 for replica number 1, which is out of range [1;31]' > + | ... > + > +-- Test big number value > +box.cfg{replication_synchro_quorum = '4294967297'} 1. This test is exactly the same as the next line, because both return true from cfg_isnumber(). So you didn't test overflow in the formula result here. > + | --- > + | - error: 'Incorrect value for option ''replication_synchro_quorum'': the value must > + | be greater than zero and less than maximal number of replicas' > + | ... > +box.cfg{replication_synchro_quorum = 4294967297} > + | --- > + | - error: 'Incorrect value for option ''replication_synchro_quorum'': the value must > + | be greater than zero and less than maximal number of replicas' > + | ... > + > +-- Timeouts for replication > +function cfg_set_pass_tmo() box.cfg{replication_synchro_timeout = 1000} end > + | --- > + | ... > +function cfg_set_fail_tmo() box.cfg{replication_synchro_timeout = 0.5} end > + | --- > + | ... > + > +-- Use canonical majority formula > +box.cfg{replication_synchro_quorum = "N/2+1"} 2. I asked it in the previous email and in the chat, but you didn't pay attention, did you? The option is never restored to the initial value in the end of the test. Timeout too. This could break the next tests running in the same test-run worker. Also you didn't drop the space in the end. Since we don't have much time, I fixed these issues, and pushed on top of the branch in a separate commit. Please, review and squash if you agree. Also the diff is below: ==================== diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result index 832c3f6e5..2acd633b2 100644 --- a/test/replication/gh-5446-qsync-eval-quorum.result +++ b/test/replication/gh-5446-qsync-eval-quorum.result @@ -10,6 +10,13 @@ box.schema.user.grant('guest', 'replication') | --- | ... +old_synchro_quorum = box.cfg.replication_synchro_quorum + | --- + | ... +old_synchro_timeout = box.cfg.replication_synchro_timeout + | --- + | ... + -- Test syntax error box.cfg{replication_synchro_quorum = "aaa"} | --- @@ -30,10 +37,10 @@ box.cfg{replication_synchro_quorum = "N-1"} | ... -- Test big number value -box.cfg{replication_synchro_quorum = '4294967297'} +box.cfg{replication_synchro_quorum = '4294967296 + 1'} | --- - | - error: 'Incorrect value for option ''replication_synchro_quorum'': the value must - | be greater than zero and less than maximal number of replicas' + | - error: 'Incorrect value for option ''replication_synchro_quorum'': the formula is + | evaluated to the quorum 1 for replica number 1, which is out of range [1;31]' | ... box.cfg{replication_synchro_quorum = 4294967297} | --- @@ -58,13 +65,10 @@ cfg_set_pass_tmo() | ... -- Create a sync space we will operate on -_ = box.schema.space.create('sync', {is_sync = true, engine = engine}) +s = box.schema.space.create('sync', {is_sync = true, engine = engine}) | --- | ... -_ = box.space.sync:create_index('pk') - | --- - | ... -s = box.space.sync +_ = s:create_index('pk') | --- | ... @@ -293,6 +297,17 @@ test_run:cmd('delete server replica6') | - true | ... +s:drop() + | --- + | ... + box.schema.user.revoke('guest', 'replication') | --- | ... + +box.cfg{ \ + replication_synchro_quorum = old_synchro_quorum, \ + replication_synchro_timeout = old_synchro_timeout, \ +} + | --- + | ... diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua index b905af3d9..e8c067246 100644 --- a/test/replication/gh-5446-qsync-eval-quorum.test.lua +++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua @@ -3,6 +3,9 @@ engine = test_run:get_cfg('engine') box.schema.user.grant('guest', 'replication') +old_synchro_quorum = box.cfg.replication_synchro_quorum +old_synchro_timeout = box.cfg.replication_synchro_timeout + -- Test syntax error box.cfg{replication_synchro_quorum = "aaa"} @@ -11,7 +14,7 @@ box.cfg{replication_synchro_quorum = "N+1"} box.cfg{replication_synchro_quorum = "N-1"} -- Test big number value -box.cfg{replication_synchro_quorum = '4294967297'} +box.cfg{replication_synchro_quorum = '4294967296 + 1'} box.cfg{replication_synchro_quorum = 4294967297} -- Timeouts for replication @@ -23,9 +26,8 @@ box.cfg{replication_synchro_quorum = "N/2+1"} cfg_set_pass_tmo() -- Create a sync space we will operate on -_ = box.schema.space.create('sync', {is_sync = true, engine = engine}) -_ = box.space.sync:create_index('pk') -s = box.space.sync +s = box.schema.space.create('sync', {is_sync = true, engine = engine}) +_ = s:create_index('pk') -- Only one master node -> 1/2 + 1 = 1 s:insert{1} -- should pass @@ -103,4 +105,11 @@ test_run:cmd('delete server replica5') test_run:cmd('stop server replica6') test_run:cmd('delete server replica6') +s:drop() + box.schema.user.revoke('guest', 'replication') + +box.cfg{ \ + replication_synchro_quorum = old_synchro_quorum, \ + replication_synchro_timeout = old_synchro_timeout, \ +}