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

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

20.04.2021 01:35, Vladislav Shpilevoy пишет:
> Good job on the patch!
>
> You didn't cover the stop_candidate call with demote flag true in the
> unit tests. I added my commit on top of this one. See it on the branch
> and below. Squash if you agree. Otherwise lets discuss.
>
> ====================

Thanks for the help!

Your changes look good, I squashed them.

> diff --git a/test/unit/raft.c b/test/unit/raft.c
> index a718ab3f4..29e48da13 100644
> --- a/test/unit/raft.c
> +++ b/test/unit/raft.c
> @@ -1270,29 +1270,50 @@ raft_test_too_long_wal_write(void)
>   static void
>   raft_test_start_stop_candidate(void)
>   {
> -	raft_start_test(4);
> +	raft_start_test(8);
>   	struct raft_node node;
>   	raft_node_create(&node);
>   
>   	raft_node_cfg_is_candidate(&node, false);
>   	raft_node_cfg_election_quorum(&node, 1);
>   
> -	raft_start_candidate(&node.raft);
> +	raft_node_start_candidate(&node);
>   	raft_run_next_event();
>   	is(node.raft.state, RAFT_STATE_LEADER, "became leader after "
> -					       "start_candidate");
> -	raft_stop_candidate(&node.raft, false);
> +	   "start candidate");
> +
> +	raft_node_stop_candidate(&node);
>   	raft_run_for(node.cfg_death_timeout);
>   	is(node.raft.state, RAFT_STATE_LEADER, "remain leader after "
> -					       "stop_candidate");
> +	   "stop candidate");
> +
> +	raft_node_demote_candidate(&node);
> +	is(node.raft.state, RAFT_STATE_FOLLOWER, "demote drops a non-candidate "
> +	   "leader to a follower");
> +
> +	/*
> +	 * Ensure the non-candidate leader is demoted when sees a new term, and
> +	 * does not try election again.
> +	 */
> +	raft_node_start_candidate(&node);
> +	raft_run_next_event();
> +	raft_node_stop_candidate(&node);
> +	is(node.raft.state, RAFT_STATE_LEADER, "non-candidate but still "
> +	   "leader");
>   
>   	is(raft_node_send_vote_request(&node,
> -		3 /* Term. */,
> +		4 /* Term. */,
>   		"{}" /* Vclock. */,
>   		2 /* Source. */
>   	), 0, "vote request from 2");
>   	is(node.raft.state, RAFT_STATE_FOLLOWER, "demote once new election "
> -						 "starts");
> +	   "starts");
> +
> +	raft_run_for(node.cfg_election_timeout * 2);
> +	is(node.raft.state, RAFT_STATE_FOLLOWER, "still follower");
> +	is(node.raft.term, 4, "still the same term");
> +
> +	raft_node_destroy(&node);
>   	raft_finish_test();
>   }
>   
> diff --git a/test/unit/raft.result b/test/unit/raft.result
> index f9a8f249b..3a3dc5dd2 100644
> --- a/test/unit/raft.result
> +++ b/test/unit/raft.result
> @@ -221,11 +221,15 @@ ok 12 - subtests
>   ok 13 - subtests
>   	*** raft_test_too_long_wal_write: done ***
>   	*** raft_test_start_stop_candidate ***
> -    1..4
> -    ok 1 - became leader after start_candidate
> -    ok 2 - remain leader after stop_candidate
> -    ok 3 - vote request from 2
> -    ok 4 - demote once new election starts
> +    1..8
> +    ok 1 - became leader after start candidate
> +    ok 2 - remain leader after stop candidate
> +    ok 3 - demote drops a non-candidate leader to a follower
> +    ok 4 - non-candidate but still leader
> +    ok 5 - vote request from 2
> +    ok 6 - demote once new election starts
> +    ok 7 - still follower
> +    ok 8 - still the same term
>   ok 14 - subtests
>   	*** raft_test_start_stop_candidate: done ***
>   	*** main_f: done ***
> diff --git a/test/unit/raft_test_utils.c b/test/unit/raft_test_utils.c
> index b8735f373..452c05c81 100644
> --- a/test/unit/raft_test_utils.c
> +++ b/test/unit/raft_test_utils.c
> @@ -387,6 +387,27 @@ raft_node_unblock(struct raft_node *node)
>   	}
>   }
>   
> +void
> +raft_node_start_candidate(struct raft_node *node)
> +{
> +	assert(raft_node_is_started(node));
> +	raft_start_candidate(&node->raft);
> +}
> +
> +void
> +raft_node_stop_candidate(struct raft_node *node)
> +{
> +	assert(raft_node_is_started(node));
> +	raft_stop_candidate(&node->raft, false);
> +}
> +
> +void
> +raft_node_demote_candidate(struct raft_node *node)
> +{
> +	assert(raft_node_is_started(node));
> +	raft_stop_candidate(&node->raft, true);
> +}
> +
>   void
>   raft_node_cfg_is_enabled(struct raft_node *node, bool value)
>   {
> diff --git a/test/unit/raft_test_utils.h b/test/unit/raft_test_utils.h
> index bc3db0c2a..5f8618716 100644
> --- a/test/unit/raft_test_utils.h
> +++ b/test/unit/raft_test_utils.h
> @@ -208,6 +208,23 @@ raft_node_block(struct raft_node *node);
>   void
>   raft_node_unblock(struct raft_node *node);
>   
> +/**
> + * Make the node candidate, and maybe start election if a leader is not known.
> + */
> +void
> +raft_node_start_candidate(struct raft_node *node);
> +
> +/**
> + * Make the node non-candidate for next elections, but if it is a leader right
> + * now, it will stay a leader.
> + */
> +void
> +raft_node_stop_candidate(struct raft_node *node);
> +
> +/** Stop the candidate and remove its leader role if present. */
> +void
> +raft_node_demote_candidate(struct raft_node *node);
> +
>   /** Configuration methods. */
>   
>   void

-- 
Serge Petrenko