Re: [Tarantool-patches] [PATCH 2/4] raft: initialize raft completely at runtime

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

Thanks for the review!

On 09.11.2020 15:59, Cyrill Gorcunov wrote:
> On Sun, Nov 08, 2020 at 07:03:55PM +0100, Vladislav Shpilevoy wrote:
>> Global raft object was created mostly at compile time with
>> constant values assigned to all raft fields. But it won't work
>> soon because raft will be moved to a separate library, and it
>> will provide methods for raft creation and destruction.
>>
>> This patch makes raft not rely on compile time initialization,
>> and moves it entirely to raft_init(). Also there are added some
>> validations via new raft_validate() function, which should ensure
>> raft is not used before creation.
>>
>> This may be hard to support, since raft is used by recovery and
>> replication, even before instance has an ID. The validation should
>> help to catch bugs related to the initialization order of these
>> subsystems.
>>
>> Part of #5303
> ...
>> @@ -1079,6 +1081,11 @@ raft_schedule_broadcast(void)
>>  void
>>  raft_init(void)
>>  {
>> +	memset(&raft, 0, sizeof(raft));
>> +	raft.state = RAFT_STATE_FOLLOWER;
>> +	raft.volatile_term = 1;
>> +	raft.term = 1;
>> +	raft.election_timeout = 5;
> 
> Please provide a separate helper instead, just like you did
> for qsync (ie limbo), say

I am not sure I understand what do you mean as 'helper'. If you
mean raft_create() for a non-global raft object, it is done
in the next patch. Because this one is not about not accessing
the global variable. It is about its initialization at runtime.

> void
> raft_init(struct raft *raft)
> {
> 	*raft = (struct raft) {
> 		.state			= RAFT_STATE_FOLLOWER,
> 		.volatile_term		= 1,
> 		.term			= 1,
> 		.election_timeout	= 5,
> 	};
> }
> 
> this way you could provide some "raft_defaults" options in future
> and simply pass it via arguments if needed. Same time this eliminate
> needless memset call, let the compiler do its work.

If you mean this = {}, then I don't mind. Except that I won't use this
extra alignment. I find it hard to follow all these spaces, when I found
a needed member name in the struct, and need to find its value now.

====================
@@ -1089,11 +1089,12 @@ raft_schedule_broadcast(void)
 void
 raft_init(void)
 {
-	memset(&raft, 0, sizeof(raft));
-	raft.state = RAFT_STATE_FOLLOWER;
-	raft.volatile_term = 1;
-	raft.term = 1;
-	raft.election_timeout = 5;
+	raft = (struct raft) {
+		.state = RAFT_STATE_FOLLOWER,
+		.volatile_term = 1,
+		.term =	1,
+		.election_timeout = 5,
+	};
 	ev_timer_init(&raft.timer, raft_sm_schedule_new_election_cb, 0, 0);
 	rlist_create(&raft.on_update);
 }
====================

>> +/**
>> + * Ensure the raft node can be used. I.e. that it is properly initialized.
>> + * Entirely for debug purposes.
>> + */
>> +static inline void
>> +raft_validate(void)
>> +{
>> +	assert(raft.state != 0);
>> +}
>> +
> 
> It should be rather called raft_assert(&raft) pointing that this won't has
> any effect on nondebug builds. On the other hands if you plan to add more tests
> here then we need to deside what to do when raft is screwed? Should we panic?

It is only a debug check. It validates the most trivial cases, which for sure
will explode in any test in case they are broken.

> Same time calling this helper on that many operations is ugly :( In next patches
> you made operations to work with box_raft instance so what we could do is instead
> of passing &box_raft we provide an inline helper which would fetch the pointer
> and verify its state instead. Say
> 
> static struct raft box_raft;
> 
> static inline struct raft *
> box_raft(void)
> {
> 	raft_assert(&box_raft);
> 	return &box_raft;
> }
> 
> 
> box_init()
> {
> 	...
> 	raft_init(&box_raft);
> 	...
> }
> 
> Hm? I'm not insisting though, just to share ideas.

Indeed. That would look better. Raft_validate() won't be useful for non-global
raft objects, because we won't be able to initialize their state as 0 at
compile time. In that way we can keep the "validation" out of src/lib/raft.