From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6A6CC4765E0 for ; Tue, 22 Dec 2020 14:14:14 +0300 (MSK) Received: by mail-lf1-f54.google.com with SMTP id h22so21538224lfu.2 for ; Tue, 22 Dec 2020 03:14:14 -0800 (PST) From: Cyrill Gorcunov Date: Tue, 22 Dec 2020 14:14:03 +0300 Message-Id: <20201222111408.48368-1-gorcunov@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [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: tml Cc: Vladislav Shpilevoy Take a look please once time permit. v2 (by Serge): - keep replication_synchro_quorum been skipped at bootstrap in load_cfg.lua - eliminate redundant say_info calls - call quorum update routine from replica_set_id/replica_clear_id - use replicaset.registered_count directly when evaluating the formula - make quorum evaluation procedure always return value in allowed range, the only error which may happen here is some syntax error or Lua evaluation errors - a test has been added v3 (by Serge, Mons, Vlad): - use replica.lua in tests - use N symbol in formula - use lua_pcall when evaluating a formula - make formula more safe itself, provide various math helpers - use box_update_replication_synchro_quorum name as a general updater from replication code - do not forget to update raft election quorum inside box_update_replication_synchro_quorum - print warns inside functions evaluator if value get out of bounds v4 (by Vlad): - when testing a formula we walk over all amount of replicas, thus we are sure that later when real evaluation takes place we won't get quorum out of bounds - improve test to make sure that when no quorum aquired the transaction doesn't pass v5 (by Vlad): - make box_check_replication_synchro_quorum to return 0|1 - in test use `s` and a shorthand for the space v6 (by Vlad): - use wait_log in test 'cause log flushing doesn't go immediately - fix potential numbers overflow when setting up quorum Vlad, I resend the whole series for one more general review. Take a look once time permit. The interdiff for v5 is below. issue https://github.com/tarantool/tarantool/issues/5446 branch gorcunov/gh-5446-eval-quorum-6 Cyrill Gorcunov (5): cfg: add cfg_isnumber helper cfg: rework box_check_replication_synchro_quorum cfg: support symbolic evaluation of replication_synchro_quorum cfg: more precise check for replication_synchro_quorum value test: add replication/gh-5446-qsync-eval-quorum.test.lua src/box/box.cc | 148 +++++++++- src/box/box.h | 1 + src/box/lua/load_cfg.lua | 2 +- src/box/replication.cc | 4 +- src/cfg.c | 9 + src/cfg.h | 6 + .../gh-5446-qsync-eval-quorum.result | 277 ++++++++++++++++++ .../gh-5446-qsync-eval-quorum.test.lua | 110 +++++++ test/replication/replica-quorum-1.lua | 1 + test/replication/replica-quorum-2.lua | 1 + test/replication/replica-quorum-3.lua | 1 + test/replication/replica-quorum-4.lua | 1 + test/replication/replica-quorum-5.lua | 1 + test/replication/replica-quorum-6.lua | 1 + 14 files changed, 552 insertions(+), 11 deletions(-) create mode 100644 test/replication/gh-5446-qsync-eval-quorum.result create mode 100644 test/replication/gh-5446-qsync-eval-quorum.test.lua create mode 120000 test/replication/replica-quorum-1.lua create mode 120000 test/replication/replica-quorum-2.lua create mode 120000 test/replication/replica-quorum-3.lua create mode 120000 test/replication/replica-quorum-4.lua create mode 120000 test/replication/replica-quorum-5.lua create mode 120000 test/replication/replica-quorum-6.lua base-commit: 28f3b2f1e845aff49048d92f9062a4dfa365bf57 -- diff --git a/src/box/box.cc b/src/box/box.cc index d3ec1faf3..b3cc45358 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -609,9 +609,9 @@ box_eval_replication_synchro_quorum(int nr_replicas) return -1; } - int quorum = -1; + int64_t quorum = -1; if (lua_isnumber(tarantool_L, -1)) - quorum = (int)lua_tonumber(tarantool_L, -1); + quorum = luaL_toint64(tarantool_L, -1); lua_pop(tarantool_L, 1); /* @@ -657,7 +657,7 @@ box_check_replication_synchro_quorum(void) return 0; } - int quorum = cfg_geti("replication_synchro_quorum"); + int64_t quorum = cfg_geti64("replication_synchro_quorum"); if (quorum <= 0 || quorum >= VCLOCK_MAX) { diag_set(ClientError, ER_CFG, "replication_synchro_quorum", "the value must be greater than zero and less than " @@ -1026,13 +1026,20 @@ box_update_replication_synchro_quorum(void) */ int value = MAX(1, replicaset.registered_count); quorum = box_eval_replication_synchro_quorum(value); - if (quorum <= 0 || quorum >= VCLOCK_MAX) - panic("failed to eval replication_synchro_quorum"); say_info("update replication_synchro_quorum = %d", quorum); } else { quorum = cfg_geti("replication_synchro_quorum"); } + /* + * This should never happen because the values were + * validated already but just to prevent from + * unexpected changes and because the value is too + * important for qsync, lets re-check (this is cheap). + */ + if (quorum <= 0 || quorum >= VCLOCK_MAX) + panic("failed to eval/fetch replication_synchro_quorum"); + replication_synchro_quorum = quorum; txn_limbo_on_parameters_change(&txn_limbo); box_raft_update_election_quorum(); diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result index 6fd31d1bc..6c3c63577 100644 --- a/test/replication/gh-5446-qsync-eval-quorum.result +++ b/test/replication/gh-5446-qsync-eval-quorum.result @@ -29,6 +29,18 @@ box.cfg{replication_synchro_quorum = "N-1"} | 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'} + | --- + | - 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' + | ... + -- Use canonical majority formula box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 } | --- @@ -36,9 +48,9 @@ box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 10 match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - set 'replication_synchro_quorum' configuration option to "N\/2+1 | ... -- Create a sync space we will operate on @@ -70,9 +82,9 @@ test_run:cmd('start server replica1 with wait=True, wait_load=True') match = 'update replication_synchro_quorum = 2' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - update replication_synchro_quorum = 2 | ... test_run:cmd('create server replica2 with rpl_master=default,\ @@ -89,9 +101,9 @@ test_run:cmd('start server replica2 with wait=True, wait_load=True') match = 'update replication_synchro_quorum = 2' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - update replication_synchro_quorum = 2 | ... test_run:cmd('create server replica3 with rpl_master=default,\ @@ -108,9 +120,9 @@ test_run:cmd('start server replica3 with wait=True, wait_load=True') match = 'update replication_synchro_quorum = 3' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - update replication_synchro_quorum = 3 | ... test_run:cmd('create server replica4 with rpl_master=default,\ @@ -127,9 +139,9 @@ test_run:cmd('start server replica4 with wait=True, wait_load=True') match = 'update replication_synchro_quorum = 3' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - update replication_synchro_quorum = 3 | ... test_run:cmd('create server replica5 with rpl_master=default,\ @@ -156,9 +168,9 @@ test_run:cmd('start server replica6 with wait=True, wait_load=True') match = 'update replication_synchro_quorum = 4' | --- | ... -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) | --- - | - true + | - update replication_synchro_quorum = 4 | ... -- 5 replicas left, the commit should pass diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua index 56269fc43..b7ce9a9ca 100644 --- a/test/replication/gh-5446-qsync-eval-quorum.test.lua +++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua @@ -10,10 +10,14 @@ box.cfg{replication_synchro_quorum = "aaa"} 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 = 4294967297} + -- Use canonical majority formula box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 } match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) -- Create a sync space we will operate on _ = box.schema.space.create('sync', {is_sync = true, engine = engine}) @@ -27,7 +31,7 @@ test_run:cmd('start server replica1 with wait=True, wait_load=True') -- 1 replica -> replication_synchro_quorum = 2/2 + 1 = 2 match = 'update replication_synchro_quorum = 2' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) test_run:cmd('create server replica2 with rpl_master=default,\ script="replication/replica-quorum-2.lua"') @@ -35,7 +39,7 @@ test_run:cmd('start server replica2 with wait=True, wait_load=True') -- 2 replicas -> replication_synchro_quorum = 3/2 + 1 = 2 match = 'update replication_synchro_quorum = 2' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) test_run:cmd('create server replica3 with rpl_master=default,\ script="replication/replica-quorum-3.lua"') @@ -43,7 +47,7 @@ test_run:cmd('start server replica3 with wait=True, wait_load=True') -- 3 replicas -> replication_synchro_quorum = 4/2 + 1 = 3 match = 'update replication_synchro_quorum = 3' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) test_run:cmd('create server replica4 with rpl_master=default,\ script="replication/replica-quorum-4.lua"') @@ -51,7 +55,7 @@ test_run:cmd('start server replica4 with wait=True, wait_load=True') -- 4 replicas -> replication_synchro_quorum = 5/2 + 1 = 3 match = 'update replication_synchro_quorum = 3' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) test_run:cmd('create server replica5 with rpl_master=default,\ script="replication/replica-quorum-5.lua"') @@ -63,7 +67,7 @@ test_run:cmd('start server replica6 with wait=True, wait_load=True') -- 6 replicas -> replication_synchro_quorum = 7/2 + 1 = 4 match = 'update replication_synchro_quorum = 4' -test_run:grep_log("default", match) ~= nil +test_run:wait_log('default', match, nil, 5) -- 5 replicas left, the commit should pass test_run:cmd('stop server replica1')