From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 E085D469719 for ; Wed, 7 Oct 2020 15:20:28 +0300 (MSK) Date: Wed, 7 Oct 2020 15:20:26 +0300 From: Sergey Ostanevich Message-ID: <20201007122026.GA2885@tarantool.org> References: <6655f0035c166540fd6a607fdab414232cb8b02c.1602016564.git.v.shpilevoy@tarantool.org> <7b4b7885-d014-ebd8-9415-45f6183268ba@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <7b4b7885-d014-ebd8-9415-45f6183268ba@tarantool.org> 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: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Hi! Thank you for the patch and review! I vote for 'election_mode' naming, since 'off' means we have election turend off on this node. The 'voter' means this node behaves in a mode it votes for others only, the 'candidate' turns into a mode of self-nomination. LGTM otherwise. Regards, Sergos On 07 окт 12:30, Serge Petrenko wrote: > > 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 >