From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 9CC3D469719 for ; Wed, 7 Oct 2020 12:30:09 +0300 (MSK) References: <6655f0035c166540fd6a607fdab414232cb8b02c.1602016564.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: <7b4b7885-d014-ebd8-9415-45f6183268ba@tarantool.org> Date: Wed, 7 Oct 2020 12:30:08 +0300 MIME-Version: 1.0 In-Reply-To: <6655f0035c166540fd6a607fdab414232cb8b02c.1602016564.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 1/1] raft: introduce election_role configuration option List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org 06.10.2020 23:36, Vladislav Shpilevoy пишет: > The new option can be one of 3 values: 'off', 'candidate', > 'voter'. It replaces 2 old options: election_is_enabled and > election_is_candidate. These flags looked strange, that it was > possible to set candidate true, but disable election at the same > time. Also it would not look good if we would ever decide to > introduce another mode like a data-less sentinel node, for > example. Just for voting. > > Anyway, the single option approach looks easier to configure and > to extend. > > - 'off' means the election is disabled on the node. It is the same > as election_is_enabled = false in the old config; > > - 'voter' means the node can vote and is never writable. The same > as election_is_enabled = true + election_is_candidate = false in > the old config; > > - 'candidate' means the node is a full-featured cluster member, > which eventually may become a leader. The same as > election_is_enabled = true + election_is_candidate = true in the > old config. > > Part of #1146 Hi! Thanks for the patch! I propose to either rename `election_role` to `election_mode` or `election_role="off"` to `election_role="none"` Other than that, LGTM. > --- > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-1146-raft-options > Issue: https://github.com/tarantool/tarantool/issues/1146 > > src/box/box.cc | 54 +++++++----------------- > src/box/box.h | 3 +- > src/box/lua/cfg.cc | 15 ++----- > src/box/lua/load_cfg.lua | 15 +++---- > test/app-tap/init_script.result | 3 +- > test/box/admin.result | 6 +-- > test/box/cfg.result | 12 ++---- > test/replication/election_basic.result | 28 ++++-------- > test/replication/election_basic.test.lua | 17 +++----- > test/replication/election_replica.lua | 3 +- > 10 files changed, 48 insertions(+), 108 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 6ec813c12..079fa8b4b 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -477,26 +477,17 @@ box_check_uri(const char *source, const char *option_name) > } > } > > -static int > -box_check_election_is_enabled(void) > +static const char * > +box_check_election_role(void) > { > - int b = cfg_getb("election_is_enabled"); > - if (b < 0) { > - diag_set(ClientError, ER_CFG, "election_is_enabled", > - "the value must be a boolean"); > + const char *role = cfg_gets("election_role"); > + if (role == NULL || (strcmp(role, "off") != 0 && > + strcmp(role, "voter") != 0 && strcmp(role, "candidate") != 0)) { > + diag_set(ClientError, ER_CFG, "election_role", "the value must " > + "be a string 'off' or 'voter' or 'candidate'"); > + return NULL; > } > - return b; > -} > - > -static int > -box_check_election_is_candidate(void) > -{ > - int b = cfg_getb("election_is_candidate"); > - if (b < 0) { > - diag_set(ClientError, ER_CFG, "election_is_candidate", > - "the value must be a boolean"); > - } > - return b; > + return role; > } > > static double > @@ -768,9 +759,7 @@ box_check_config(void) > box_check_uri(cfg_gets("listen"), "listen"); > box_check_instance_uuid(&uuid); > box_check_replicaset_uuid(&uuid); > - if (box_check_election_is_enabled() < 0) > - diag_raise(); > - if (box_check_election_is_candidate() < 0) > + if (box_check_election_role() == NULL) > diag_raise(); > if (box_check_election_timeout() < 0) > diag_raise(); > @@ -797,22 +786,13 @@ box_check_config(void) > } > > int > -box_set_election_is_enabled(void) > +box_set_election_role(void) > { > - int b = box_check_election_is_enabled(); > - if (b < 0) > + const char *role = box_check_election_role(); > + if (role == NULL) > return -1; > - raft_cfg_is_enabled(b); > - return 0; > -} > - > -int > -box_set_election_is_candidate(void) > -{ > - int b = box_check_election_is_candidate(); > - if (b < 0) > - return -1; > - raft_cfg_is_candidate(b); > + raft_cfg_is_candidate(strcmp(role, "candidate") == 0); > + raft_cfg_is_enabled(strcmp(role, "off") != 0); > return 0; > } > > @@ -2786,15 +2766,13 @@ box_cfg_xc(void) > * election-enabled node without election actually enabled leading to > * disconnect. > */ > - if (box_set_election_is_candidate() != 0) > - diag_raise(); > if (box_set_election_timeout() != 0) > diag_raise(); > /* > * Election is enabled last. So as all the parameters are installed by > * that time. > */ > - if (box_set_election_is_enabled() != 0) > + if (box_set_election_role() != 0) > diag_raise(); > > title("running"); > diff --git a/src/box/box.h b/src/box/box.h > index 45ff8bbbf..ca66ecff6 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -245,8 +245,7 @@ void box_set_vinyl_memory(void); > void box_set_vinyl_max_tuple_size(void); > void box_set_vinyl_cache(void); > void box_set_vinyl_timeout(void); > -int box_set_election_is_enabled(void); > -int box_set_election_is_candidate(void); > +int box_set_election_role(void); > int box_set_election_timeout(void); > void box_set_replication_timeout(void); > void box_set_replication_connect_timeout(void); > diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc > index bbb92f038..94a7eea90 100644 > --- a/src/box/lua/cfg.cc > +++ b/src/box/lua/cfg.cc > @@ -270,17 +270,9 @@ lbox_cfg_set_worker_pool_threads(struct lua_State *L) > } > > static int > -lbox_cfg_set_election_is_enabled(struct lua_State *L) > +lbox_cfg_set_election_role(struct lua_State *L) > { > - if (box_set_election_is_enabled() != 0) > - luaT_error(L); > - return 0; > -} > - > -static int > -lbox_cfg_set_election_is_candidate(struct lua_State *L) > -{ > - if (box_set_election_is_candidate() != 0) > + if (box_set_election_role() != 0) > luaT_error(L); > return 0; > } > @@ -406,8 +398,7 @@ box_lua_cfg_init(struct lua_State *L) > {"cfg_set_vinyl_max_tuple_size", lbox_cfg_set_vinyl_max_tuple_size}, > {"cfg_set_vinyl_cache", lbox_cfg_set_vinyl_cache}, > {"cfg_set_vinyl_timeout", lbox_cfg_set_vinyl_timeout}, > - {"cfg_set_election_is_enabled", lbox_cfg_set_election_is_enabled}, > - {"cfg_set_election_is_candidate", lbox_cfg_set_election_is_candidate}, > + {"cfg_set_election_role", lbox_cfg_set_election_role}, > {"cfg_set_election_timeout", lbox_cfg_set_election_timeout}, > {"cfg_set_replication_timeout", lbox_cfg_set_replication_timeout}, > {"cfg_set_replication_connect_quorum", lbox_cfg_set_replication_connect_quorum}, > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index d558e7ac9..b2c3ce188 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -87,8 +87,7 @@ local default_cfg = { > checkpoint_wal_threshold = 1e18, > checkpoint_count = 2, > worker_pool_threads = 4, > - election_is_enabled = false, > - election_is_candidate = true, > + election_role = 'off', > election_timeout = 5, > replication_timeout = 1, > replication_sync_lag = 10, > @@ -168,8 +167,7 @@ local template_cfg = { > hot_standby = 'boolean', > memtx_use_mvcc_engine = 'boolean', > worker_pool_threads = 'number', > - election_is_enabled = 'boolean', > - election_is_candidate = 'boolean', > + election_role = 'string', > election_timeout = 'number', > replication_timeout = 'number', > replication_sync_lag = 'number', > @@ -287,8 +285,7 @@ local dynamic_cfg = { > require('title').update(box.cfg.custom_proc_title) > end, > force_recovery = function() end, > - election_is_enabled = private.cfg_set_election_is_enabled, > - election_is_candidate = private.cfg_set_election_is_candidate, > + election_role = private.cfg_set_election_role, > election_timeout = private.cfg_set_election_timeout, > replication_timeout = private.cfg_set_replication_timeout, > replication_connect_timeout = private.cfg_set_replication_connect_timeout, > @@ -344,8 +341,7 @@ local dynamic_cfg_order = { > -- the new one. This should be fixed when box.cfg is able to > -- apply some parameters together and atomically. > replication_anon = 250, > - election_is_enabled = 300, > - election_is_candidate = 310, > + election_role = 300, > election_timeout = 320, > } > > @@ -364,8 +360,7 @@ local dynamic_cfg_skip_at_load = { > vinyl_cache = true, > vinyl_timeout = true, > too_long_threshold = true, > - election_is_enabled = true, > - election_is_candidate = true, > + election_role = true, > election_timeout = true, > replication = true, > replication_timeout = true, > diff --git a/test/app-tap/init_script.result b/test/app-tap/init_script.result > index d8969278b..55b739bc6 100644 > --- a/test/app-tap/init_script.result > +++ b/test/app-tap/init_script.result > @@ -8,8 +8,7 @@ checkpoint_count:2 > checkpoint_interval:3600 > checkpoint_wal_threshold:1e+18 > coredump:false > -election_is_candidate:true > -election_is_enabled:false > +election_role:off > election_timeout:5 > feedback_enabled:true > feedback_host:https://feedback.tarantool.io > diff --git a/test/box/admin.result b/test/box/admin.result > index 52b62356f..383ffec83 100644 > --- a/test/box/admin.result > +++ b/test/box/admin.result > @@ -37,10 +37,8 @@ cfg_filter(box.cfg) > - 1000000000000000000 > - - coredump > - false > - - - election_is_candidate > - - true > - - - election_is_enabled > - - false > + - - election_role > + - off > - - election_timeout > - 5 > - - feedback_enabled > diff --git a/test/box/cfg.result b/test/box/cfg.result > index f19f4bff7..10c505593 100644 > --- a/test/box/cfg.result > +++ b/test/box/cfg.result > @@ -25,10 +25,8 @@ cfg_filter(box.cfg) > | - 1000000000000000000 > | - - coredump > | - false > - | - - election_is_candidate > - | - true > - | - - election_is_enabled > - | - false > + | - - election_role > + | - off > | - - election_timeout > | - 5 > | - - feedback_enabled > @@ -140,10 +138,8 @@ cfg_filter(box.cfg) > | - 1000000000000000000 > | - - coredump > | - false > - | - - election_is_candidate > - | - true > - | - - election_is_enabled > - | - false > + | - - election_role > + | - off > | - - election_timeout > | - 5 > | - - feedback_enabled > diff --git a/test/replication/election_basic.result b/test/replication/election_basic.result > index e59386f90..28143124b 100644 > --- a/test/replication/election_basic.result > +++ b/test/replication/election_basic.result > @@ -11,25 +11,19 @@ old_election_timeout = box.cfg_election_timeout > | ... > > -- Election is turned off by default. > -assert(not box.cfg.election_is_enabled) > - | --- > - | - true > - | ... > --- Is candidate by default. Although it does not matter, until election is > --- turned on. > -assert(box.cfg.election_is_candidate) > +assert(box.cfg.election_role == 'off') > | --- > | - true > | ... > -- Ensure election options are validated. > -box.cfg{election_is_enabled = 100} > +box.cfg{election_role = 100} > | --- > - | - error: 'Incorrect value for option ''election_is_enabled'': should be of type boolean' > + | - error: 'Incorrect value for option ''election_role'': should be of type string' > | ... > -box.cfg{election_is_candidate = 100} > +box.cfg{election_role = '100'} > | --- > - | - error: 'Incorrect value for option ''election_is_candidate'': should be of type > - | boolean' > + | - error: 'Incorrect value for option ''election_role'': the value must be a string > + | ''off'' or ''voter'' or ''candidate''' > | ... > box.cfg{election_timeout = -1} > | --- > @@ -64,10 +58,7 @@ assert(not box.info.ro) > | ... > > -- Turned on election blocks writes until the instance becomes a leader. > -box.cfg{election_is_candidate = false} > - | --- > - | ... > -box.cfg{election_is_enabled = true} > +box.cfg{election_role = 'voter'} > | --- > | ... > assert(box.info.election.state == 'follower') > @@ -97,7 +88,7 @@ assert(box.info.election.leader == 0) > box.cfg{election_timeout = 1000} > | --- > | ... > -box.cfg{election_is_candidate = true} > +box.cfg{election_role = 'candidate'} > | --- > | ... > test_run:wait_cond(function() return box.info.election.state == 'leader' end) > @@ -118,8 +109,7 @@ assert(box.info.election.leader == box.info.id) > | ... > > box.cfg{ \ > - election_is_enabled = false, \ > - election_is_candidate = true, \ > + election_role = 'off', \ > election_timeout = old_election_timeout \ > } > | --- > diff --git a/test/replication/election_basic.test.lua b/test/replication/election_basic.test.lua > index 506d5ec4e..81f587866 100644 > --- a/test/replication/election_basic.test.lua > +++ b/test/replication/election_basic.test.lua > @@ -6,13 +6,10 @@ test_run = require('test_run').new() > old_election_timeout = box.cfg_election_timeout > > -- Election is turned off by default. > -assert(not box.cfg.election_is_enabled) > --- Is candidate by default. Although it does not matter, until election is > --- turned on. > -assert(box.cfg.election_is_candidate) > +assert(box.cfg.election_role == 'off') > -- Ensure election options are validated. > -box.cfg{election_is_enabled = 100} > -box.cfg{election_is_candidate = 100} > +box.cfg{election_role = 100} > +box.cfg{election_role = '100'} > box.cfg{election_timeout = -1} > box.cfg{election_timeout = 0} > > @@ -25,8 +22,7 @@ assert(box.info.election.leader == 0) > assert(not box.info.ro) > > -- Turned on election blocks writes until the instance becomes a leader. > -box.cfg{election_is_candidate = false} > -box.cfg{election_is_enabled = true} > +box.cfg{election_role = 'voter'} > assert(box.info.election.state == 'follower') > assert(box.info.ro) > -- Term is not changed, because the instance can't be a candidate, > @@ -37,15 +33,14 @@ assert(box.info.election.leader == 0) > > -- Candidate instance votes immediately, if sees no leader. > box.cfg{election_timeout = 1000} > -box.cfg{election_is_candidate = true} > +box.cfg{election_role = 'candidate'} > test_run:wait_cond(function() return box.info.election.state == 'leader' end) > assert(box.info.election.term > term) > assert(box.info.election.vote == box.info.id) > assert(box.info.election.leader == box.info.id) > > box.cfg{ \ > - election_is_enabled = false, \ > - election_is_candidate = true, \ > + election_role = 'off', \ > election_timeout = old_election_timeout \ > } > > diff --git a/test/replication/election_replica.lua b/test/replication/election_replica.lua > index b7d1aebe7..96a01245b 100644 > --- a/test/replication/election_replica.lua > +++ b/test/replication/election_replica.lua > @@ -19,8 +19,7 @@ box.cfg({ > instance_uri(3), > }, > replication_timeout = 0.1, > - election_is_enabled = true, > - election_is_candidate = true, > + election_role = 'candidate', > election_timeout = ELECTION_TIMEOUT, > replication_synchro_quorum = SYNCHRO_QUORUM, > replication_synchro_timeout = 0.1, -- Serge Petrenko