Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich <sergos@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/1] raft: introduce election_role configuration option
Date: Wed, 7 Oct 2020 15:20:26 +0300	[thread overview]
Message-ID: <20201007122026.GA2885@tarantool.org> (raw)
In-Reply-To: <7b4b7885-d014-ebd8-9415-45f6183268ba@tarantool.org>

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
> 

  reply	other threads:[~2020-10-07 12:20 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-06 20:36 Vladislav Shpilevoy
2020-10-07  9:30 ` Serge Petrenko
2020-10-07 12:20   ` Sergey Ostanevich [this message]
2020-10-07 22:29     ` Vladislav Shpilevoy
2020-10-12 20:16 ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201007122026.GA2885@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/1] raft: introduce election_role configuration option' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox