Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum

[Serge Petrenko <sergepetrenko@tarantool.org>]

19.11.2020 22:40, Cyrill Gorcunov пишет:
> Prepare scaffolds to evaluate replication_synchro_quorum,
> since we don't yet update the undeneath engines we accept
> and verify the formulas but refuse to proceed.
>
> In next patch we will support dynamic evaluation of
> the quorum number.
>
> Part-of #5446


Hi! Thanks for the patch!

Please find my comments below.


>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/box.cc           | 82 +++++++++++++++++++++++++++++++++++++++-
>   src/box/box.h            |  1 +
>   src/box/lua/load_cfg.lua |  3 +-
>   3 files changed, 83 insertions(+), 3 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 5fcf28cb3..5f7ddfa99 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -562,10 +562,90 @@ box_check_replication_sync_lag(void)
>   	return lag;
>   }
>   
> +/**
> + * Evaluate replicaion syncro quorum number from a formula.
> + */
> +int
> +eval_replication_synchro_quorum(int nr_replicas)
> +{
> +	const char fmt[] =
> +		"local f, err = loadstring(\"return (%s)\")\n"
> +		"if not f then return 'failed to load \"%s\"' end\n"
> +		"setfenv(f, { n = %d })\n"
> +		"local ok, res = pcall(f)\n"
> +		"if not ok then return res end\n"
> +		"return math.floor(res)\n";
> +	char buf[512];
> +	int value = -1;
> +
> +	errno = 0;
> +
> +	const char *expr = cfg_gets("replication_synchro_quorum");
> +	size_t ret = snprintf(buf, sizeof(buf), fmt, expr,
> +			      expr, nr_replicas);
> +	if (ret >= sizeof(buf)) {
> +		errno = EINVAL;
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "the expression is too big");
> +		return -1;
> +	}
> +
> +	luaL_loadstring(tarantool_L, buf);
> +	lua_call(tarantool_L, 0, 1);
> +
> +	if (lua_isnumber(tarantool_L, -1)) {
> +		value = (int)lua_tonumber(tarantool_L, -1);
> +	} else {
> +		assert(lua_isstring(tarantool_L, -1));
> +		errno = EINVAL;
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 lua_tostring(tarantool_L, -1));
> +	}
> +	lua_pop(tarantool_L, 1);
> +	return value;
> +}

Better make  the function return -1 on an error, and when the function 
evaluates to something negative,
log the eval result and return 1. This way you won't need errno.

> +
>   static int
>   box_check_replication_synchro_quorum(void)
>   {
> -	int quorum = cfg_geti("replication_synchro_quorum");
> +	int quorum = 0;
> +
> +	if (!cfg_isnumber("replication_synchro_quorum")) {
> +		/*
> +		 * When validating a formula it must return a
> +		 * positive value for a single node and maximum
> +		 * possible replicas because the quorum will be
> +		 * evaluated on each new replica registration,
> +		 * starting from a single node.
> +		 */
> +		int v[] = {1, VCLOCK_MAX-1};
> +		for (size_t i = 0; i < lengthof(v); i++) {
> +			quorum = eval_replication_synchro_quorum(v[i]);
> +			if (quorum < 0 && errno == EINVAL)
> +				return -1;
> +		}

You should either check every possible value, from 1 to VCLOCK_MAX - 1,
to make sure, say, that no division by zero is involved for some input.

or check a single value, say, 1, or 2 or whatever, to make sure that the
expression in replication_synchro_quorum is  at least valid Lua code.

Why check only min and max inputs?

> +
> +		/*
> +		 * Once syntax is valid we should pass the real
> +		 * default value from replication module itself
> +		 * to evaluate the actual value to use.
> +		 */
> +		int value = replication_synchro_quorum;
> +		quorum = eval_replication_synchro_quorum(value);
> +		/*
> +		 * FIXME: Until we get full support.
> +		 */
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "symbolic evaluation is not yet supported");
> +		diag_log();
> +		quorum = -1;
> +	} else {
> +		quorum = cfg_geti("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 "
> diff --git a/src/box/box.h b/src/box/box.h
> index b47a220b7..8f438faab 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -252,6 +252,7 @@ void box_set_replication_connect_timeout(void);
>   void box_set_replication_connect_quorum(void);
>   void box_set_replication_sync_lag(void);
>   int box_set_replication_synchro_quorum(void);
> +int eval_replication_synchro_quorum(int nr_replicas);
>   int box_set_replication_synchro_timeout(void);
>   void box_set_replication_sync_timeout(void);
>   void box_set_replication_skip_conflict(void);
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 76e2e92c2..26725e08d 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -172,7 +172,7 @@ local template_cfg = {
>       replication_timeout = 'number',
>       replication_sync_lag = 'number',
>       replication_sync_timeout = 'number',
> -    replication_synchro_quorum = 'number',
> +    replication_synchro_quorum = 'string, number',
>       replication_synchro_timeout = 'number',
>       replication_connect_timeout = 'number',
>       replication_connect_quorum = 'number',
> @@ -368,7 +368,6 @@ local dynamic_cfg_skip_at_load = {
>       replication_connect_quorum = true,
>       replication_sync_lag    = true,
>       replication_sync_timeout = true,
> -    replication_synchro_quorum = true,

You shouldn't remove replication_synchro_quorum from here.
This table  lists the options which are set directly from `box_cfg` in 
specific order.
I.e. not lua calls  box_cfg_set_..., but box_cfg() itself does. But only 
on the first box.cfg call.

>       replication_synchro_timeout = true,
>       replication_skip_conflict = true,
>       replication_anon        = true,

-- 
Serge Petrenko