From: Serge Petrenko <sergepetrenko@tarantool.org> To: Cyrill Gorcunov <gorcunov@gmail.com> Cc: tml <tarantool-patches@dev.tarantool.org>, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Subject: Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum Date: Fri, 20 Nov 2020 14:56:12 +0300 [thread overview] Message-ID: <b7652b68-a260-a49a-dd85-355269461be5@tarantool.org> (raw) In-Reply-To: <20201120113454.GB875895@grain> 20.11.2020 14:34, Cyrill Gorcunov пишет: > On Fri, Nov 20, 2020 at 01:32:42PM +0300, Serge Petrenko wrote: >>> +/** >>> + * 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. > We need to distinguish wrong syntax and wrong evaluated values somehow > which means using -1 for anything wont work here. > > - wrong syntax (which includes too long formula): -1 && errno > - on correct syntax we simply return the result the caller has > to check if returned value fits the allowed range. > > Or I'm missing something. I was thinking that we're gonna return something like max(1, min(value, 31)) so that any evaluated number is correct. Lets better discuss this verbally then. > >> 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. > That's the good point. Another question if we should allow formulas like > n-2, and while n <= 2 assume the quorum to be 1? Ie max(1, eval(n)) Yes, that's what I was speaking about above. So that when the formula may be evaluated correctly (i.e. without division by zero or syntax errors) its result will automatically be correct. > >> 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? > Lets consider the evaluation a bit closer. Denote arguments as x, thus > we will have x ∈ N, where are x's are strictly ordered, thus to get > f(x) < 0 between min/max values we need to use nonlinear f. I must confess > I don't know what is allowed inside Lua evaluator and really don't like > that we're allowing to pass completely arbitrary formula here. Moreover > what would happen if f would be using some weird data so that if will be > linear on bootstrap procedure but then switched to something more complex? > All this arbitrary evaluation looks very very fishy and I bet there will > be problems because this is the data we do not control completely. I guess we shouldn't be this crazy about what is allowed in this formula and what's not. If a user has access to box.cfg{}, he may evaluate any expression he wishes anyway. Anyway, this is subject of a verbal discussion. > > Back to the former question -- initially I assume the f gonna be linear > and eval in min/max will be enough. But of course this is not correct. > > You know I can pass all N's here but still this doesn't guarantee anything :( > That's why I'm for more strict rules here: > > - allow some symbolic names such as > "all" -> (alias for f(x) = n) > "canonical" -> (alias for f(x) = n/2 + 1) Sounds good to me. AFAIR others were agains it, though. > > - implement own syntax analizyer/evaluator which would allow only > basic operation {+,/} over the set of {n} (which will give us > a linear functioni by definition). This of course will require > a way more code so not right now. > >>> @@ -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. > No, this table is to _skip_ evaluation on bootup. But we have to verify > the default value to evaluate. Yes, that's what I'm talking about. Even if the cfg option from this list is 'skipped' in lua, it's referenced directly from box_cfg_xc(). Othervise the `box_cfg_set_...` will be called twice. Once from box_cfg_xc(), second time from this lua code. To be more verbose, all the setters from dynamic_cfg_skip_at_load are called on bootstrap. But from box_cfg_xc() in C, not from Lua. If you remove an entry from dynamic_cfg_skip_at_load, the corresponding setter will be called twice. > >> I.e. not lua calls box_cfg_set_..., but box_cfg() itself does. But only on >> the first box.cfg call. -- Serge Petrenko
next prev parent reply other threads:[~2020-11-20 11:56 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-19 19:40 [Tarantool-patches] [RFC 0/4] qsync: evaluate replication_synchro_quorum dynamically Cyrill Gorcunov 2020-11-19 19:40 ` [Tarantool-patches] [RFC 1/4] cfg: add cfg_isnumber helper Cyrill Gorcunov 2020-11-20 9:53 ` Serge Petrenko 2020-11-19 19:40 ` [Tarantool-patches] [RFC 2/4] qsync: move synchro quorum update to separate routine Cyrill Gorcunov 2020-11-20 10:06 ` Serge Petrenko 2020-11-20 11:01 ` Cyrill Gorcunov 2020-11-20 11:39 ` Serge Petrenko 2020-11-20 11:47 ` Cyrill Gorcunov 2020-11-19 19:40 ` [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum Cyrill Gorcunov 2020-11-20 10:32 ` Serge Petrenko 2020-11-20 11:34 ` Cyrill Gorcunov 2020-11-20 11:56 ` Serge Petrenko [this message] 2020-11-20 12:14 ` Cyrill Gorcunov 2020-11-26 14:38 ` Mons Anderson 2020-11-26 14:44 ` Cyrill Gorcunov 2020-11-26 16:01 ` Mons Anderson 2020-11-19 19:41 ` [Tarantool-patches] [RFC 4/4] qsync: allow to specify replication_synchro_quorum as a formula Cyrill Gorcunov 2020-11-20 10:50 ` Serge Petrenko 2020-11-20 12:01 ` Cyrill Gorcunov 2020-11-20 12:41 ` Serge Petrenko 2020-11-20 15:00 ` Cyrill Gorcunov
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=b7652b68-a260-a49a-dd85-355269461be5@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum' \ /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