Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>>>  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;