Re: [Tarantool-patches] [PATCH v4 09/12] raft: introduce raft_start/stop_candidate

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

17.04.2021 01:23, Vladislav Shpilevoy пишет:
> Thanks for working on this!
>
> See 3 comments below.

Thanks for  the review!

>
>>   src/lib/raft/raft.c   | 83 ++++++++++++++++++++++++++++---------------
>>   src/lib/raft/raft.h   | 13 +++++++
>>   test/unit/raft.c      | 33 +++++++++++++++--
>>   test/unit/raft.result | 10 +++++-
>>   4 files changed, 108 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
>> index e9ce8cade..b21693642 100644
>> --- a/src/lib/raft/raft.c
>> +++ b/src/lib/raft/raft.c
>> @@ -848,38 +848,65 @@ raft_cfg_is_enabled(struct raft *raft, bool is_enabled)
> <...>
>
>> +
>> +void
>> +raft_stop_candidate(struct raft *raft, bool demote)
> 1. For flags we usually use 'is', 'do', 'has' and similar prefixes.

Ok, let it be do_demote then.

>
>> +{
>> +	if (!raft->is_candidate)
>> +		return;
>> +	raft->is_candidate = false;
>> +	if (raft->state != RAFT_STATE_LEADER) {
>> +		/* Do not wait for anything while being a voter. */
>> +		raft_ev_timer_stop(raft_loop(), &raft->timer);
>> +	}
>> +	if (raft->state != RAFT_STATE_FOLLOWER) {
>> +		if (raft->state == RAFT_STATE_LEADER) {
>> +			if (!demote) {
>> +				/*
>> +				 * Remain leader until someone
>> +				 * triggers new elections.
>> +				 */
>> +				return;
>> +			}
>> +			raft->leader = 0;
>>   		}
>> +		raft->state = RAFT_STATE_FOLLOWER;
>> +		/* State is visible and changed - broadcast. */
>> +		raft_schedule_broadcast(raft);
>>   	}
>>   }
>>   
>> diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h
>> index a5f7e08d9..69dec63c6 100644
>> --- a/src/lib/raft/raft.h
>> +++ b/src/lib/raft/raft.h
>> @@ -327,6 +327,19 @@ raft_cfg_is_enabled(struct raft *raft, bool is_enabled);
>>   void
>>   raft_cfg_is_candidate(struct raft *raft, bool is_candidate);
>>   
>> +/**
>> + * Make the instance a candidate.
>> + */
>> +void
>> +raft_start_candidate(struct raft *raft);
>> +
>> +/**
>> + * Make the instance stop taking part in new elections.
>> + * @param demote whether to stop being a leader immediately or not.
>> + */
>> +void
>> +raft_stop_candidate(struct  raft *raft, bool demote);
> 2. Double whitespace after 'struct'.

Fixed.

>
>> +
>>   /** Configure Raft leader election timeout. */
>>   void
>>   raft_cfg_election_timeout(struct raft *raft, double timeout);
>> diff --git a/test/unit/raft.c b/test/unit/raft.c
>> index 0306cefcd..575886932 100644
>> --- a/test/unit/raft.c
>> +++ b/test/unit/raft.c
>> @@ -1296,15 +1296,43 @@ raft_test_term_filter(void)
>>   	ok(!raft_is_node_outdated(&node.raft, 3), "node doesn't become "
>>   						  "outdated");
>>   
>> -
> 3. This probably should be in the previous commit.

This became obsolete after I squashed your commit, but
thanks for noticing!

>
>>   	raft_node_destroy(&node);
>>   	raft_finish_test();
>>   }

=================================

diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index 874e9157e..3f900db9a 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -865,7 +865,7 @@ raft_start_candidate(struct raft *raft)
         assert(raft->state != RAFT_STATE_CANDIDATE);
         /*
          * May still be the leader after raft_stop_candidate
-        * with demote = false.
+        * with do_demote = false.
          */
         if (raft->state == RAFT_STATE_LEADER)
                 return;
@@ -884,7 +884,7 @@ raft_start_candidate(struct raft *raft)
  }

  void
-raft_stop_candidate(struct raft *raft, bool demote)
+raft_stop_candidate(struct raft *raft, bool do_demote)
  {
         if (!raft->is_candidate)
                 return;
@@ -895,7 +895,7 @@ raft_stop_candidate(struct raft *raft, bool demote)
         }
         if (raft->state != RAFT_STATE_FOLLOWER) {
                 if (raft->state == RAFT_STATE_LEADER) {
-                       if (!demote) {
+                       if (!do_demote) {
                                 /*
                                  * Remain leader until someone
                                  * triggers new elections.
diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h
index f7bc205d2..a8da564b0 100644
--- a/src/lib/raft/raft.h
+++ b/src/lib/raft/raft.h
@@ -289,10 +289,10 @@ raft_start_candidate(struct raft *raft);

  /**
   * Make the instance stop taking part in new elections.
- * @param demote whether to stop being a leader immediately or not.
+ * @param do_demote whether to stop being a leader immediately or not.
   */
  void
-raft_stop_candidate(struct  raft *raft, bool demote);
+raft_stop_candidate(struct raft *raft, bool do_demote);

  /** Configure Raft leader election timeout. */
  void

-- 
Serge Petrenko