Re: [Tarantool-patches] [PATCH v6 0/5] qsync: evaluate replication_synchro_quorum dynamically

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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,                          \
+}