From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 8A9BF45C304 for ; Sun, 20 Dec 2020 20:01:27 +0300 (MSK) References: <20201214113935.1040421-1-gorcunov@gmail.com> <20201214113935.1040421-3-gorcunov@gmail.com> <20201218072529.GF14556@grain> From: Vladislav Shpilevoy Message-ID: <47af00c0-5f40-533c-f51c-84d16c609c11@tarantool.org> Date: Sun, 20 Dec 2020 18:01:25 +0100 MIME-Version: 1.0 In-Reply-To: <20201218072529.GF14556@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: Mons Anderson , tml >>> static int >>> box_check_replication_synchro_quorum(void) >>> { >>> - int quorum = cfg_geti("replication_synchro_quorum"); >>> + int quorum = 0; >>> + >>> + if (!cfg_isnumber("replication_synchro_quorum")) { >>> + /* >>> + * The formula uses symbolic name 'N' as >>> + * a number of currently registered replicas. >>> + * >>> + * When we're in "checking" mode we should walk >>> + * over all possible number of replicas to make >>> + * sure the formula is correct. >>> + * >>> + * Note that currently VCLOCK_MAX is pretty small >>> + * value but if we gonna increase this limit make >>> + * sure that the cycle won't take too much time. >>> + */ >>> + for (int i = 1; i < VCLOCK_MAX; i++) { >>> + quorum = box_eval_replication_synchro_quorum(i); >>> + if (quorum < 0) >>> + return -1; >>> + } >>> + /* >>> + * Just to make clear the number we return here doesn't >>> + * have any special meaning, only errors are matter. >>> + * The real value is dynamic and will be updated on demand. >>> + */ >> >> 3. Wtf? This function before your patch was supposed to return the >> new quorum value. Like all cfg 'check()' functions. If it can't do that >> now (but it can - just evaluate with the current number of replicas), >> then the function must return only 0 or -1, like all 'binary result' >> functions. Now it simply returns some random number in case the quorum >> is an expression. > > As I pointer in the comment there is no special meaning in the return > value, since we update the quorum on demand. Moreover once we manage > to validate the formula we call box_update_replication_synchro_quorum > which reevaluates the quorum with current number of replicas, thus > to not make same work twice I will return 0 here. I saw the comment and I understood it. But it does not mean the function can now return random values. It does not matter if you return 0 or 1 or whatever else. It still can't be used for anything useful except check < 0. That makes the function result confusing and even useless. Previously quorum value was returned *and used as quorum value*. Now it is used only for < 0 check. Hence, why do you need to return anything except -1 and 0? For example, look at box_check_replication_synchro_timeout(). It returns *timeout value*, which is used to set replication_synchro_timeout. box_set_replication_synchro_quorum() before your patch did the same with box_check_replication_synchro_quorum(). Now it uses check() result only to compare it with < 0, which is *confusing* - why would it need to return anything but -1 and 0 then? Here is what I want to see: ==================== diff --git a/src/box/box.cc b/src/box/box.cc index ff5c27743..751ec4733 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -640,8 +640,6 @@ box_eval_replication_synchro_quorum(int nr_replicas) static int box_check_replication_synchro_quorum(void) { - int quorum = 0; - if (!cfg_isnumber("replication_synchro_quorum")) { /* * The formula uses symbolic name 'N' as @@ -656,8 +654,7 @@ box_check_replication_synchro_quorum(void) * sure that the cycle won't take too much time. */ for (int i = 1; i < VCLOCK_MAX; i++) { - quorum = box_eval_replication_synchro_quorum(i); - if (quorum < 0) + if (box_eval_replication_synchro_quorum(i) < 0) return -1; } /* @@ -666,17 +663,15 @@ box_check_replication_synchro_quorum(void) * The real value is dynamic and will be updated on demand. */ return 0; - } else { - quorum = cfg_geti("replication_synchro_quorum"); } - + int 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 " "maximal number of replicas"); return -1; } - return quorum; + return 0; } static double @@ -877,7 +872,7 @@ box_check_config(void) box_check_replication_connect_timeout(); box_check_replication_connect_quorum(); box_check_replication_sync_lag(); - if (box_check_replication_synchro_quorum() < 0) + if (box_check_replication_synchro_quorum() != 0) diag_raise(); if (box_check_replication_synchro_timeout() < 0) diag_raise(); @@ -1057,8 +1052,7 @@ box_update_replication_synchro_quorum(void) int box_set_replication_synchro_quorum(void) { - int value = box_check_replication_synchro_quorum(); - if (value < 0) + if (box_check_replication_synchro_quorum() != 0) return -1; box_update_replication_synchro_quorum(); return 0;