[Tarantool-patches] [PATCH 1/1] raft: introduce election_role configuration option
Sergey Ostanevich
sergos at tarantool.org
Wed Oct 7 15:20:26 MSK 2020
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
>
More information about the Tarantool-patches
mailing list