[tarantool-patches] Re: [PATCH 5/5] swim: introduce dissemination component

[Vladislav Shpilevoy]

Kostja pushed some comment updates. I copy-pasted
them below and fixed or reverted some of them with
an explanation why.

Please, do not forget to answer on this email and others
in the thread with an LGTM or new review fixes/comments.

> commit ce5b71857310b7c692d6d5eb8aae3f2bc2604320
> Author: Konstantin Osipov <kostja at tarantool.org>
> Date:   Mon Apr 8 17:25:13 2019 +0300
> 
>     swim: update comments, use singular in method names.
> 
> diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c
> index 8eac102c8..215960911 100644
> --- a/src/lib/swim/swim.c
> +++ b/src/lib/swim/swim.c
> @@ -274,22 +274,47 @@ struct swim_member {
>  	 *                 Dissemination component
>  	 *
>  	 * Dissemination component sends events. Event is a
> -	 * notification about member status update. So formally,
> -	 * this structure already has all the needed attributes.
> -	 * But also an event somehow should be sent to all members
> -	 * at least once according to SWIM, so it requires
> -	 * something like TTL for each type of event, which gets
> -	 * decremented on each send. And a member can not be
> -	 * removed from the global table until it gets dead and
> -	 * its status TTLs is 0, so as to allow other members
> -	 * learn its dead status.
> +	 * notification about some member state update. We maintain
> +	 * a different event type for each significant member
> +	 * attribute - status, incarnation, etc to not send entire
> +	 * member state each time any member attribute changes.
> +	 * The state of an event is stored within

My update:

==================================================================
 	 * Dissemination component sends events. Event is a
-	 * notification about some member state update. We maintain
-	 * a different event type for each significant member
-	 * attribute - status, incarnation, etc to not send entire
+	 * notification about some member state update. The member
+	 * maintains a different event type for each significant
+	 * attribute - status, incarnation, etc not to send entire
 	 * member state each time any member attribute changes.
-	 * The state of an event is stored within
==================================================================

1) Note, that I deliberately avoid personalization in the comments.
IMO, too extensive usage of 'we', 'us' and other pronouns harms
readability. Sometimes it is not obvious what do you mean by
'we'/'us', and it is better to say explicitly 'instance', 'member'
etc. After all, it is not a chat or face-to-face conversation. It
is a document.

2) In English 'to not do ...' is wrong, it is rather Russian
construction. In English they use 'not to do ...'.

3) I dropped 'stored within' because obviously that sentence
is truncated, and makes no sense.

> +	 *
> +	 * According to SWIM, an event should be sent to
> +	 * all members at least once. It'd be silly to continue
> +	 * sending an old event if a member attribute has changed
> +	 * since the beginning of the round. To this end
> +	 * we also maintain TTL (time-to-live) counter
> +	 * associated with each event type.

My update:

==================================================================
-	 * According to SWIM, an event should be sent to
-	 * all members at least once. It'd be silly to continue
-	 * sending an old event if a member attribute has changed
-	 * since the beginning of the round. To this end
-	 * we also maintain TTL (time-to-live) counter
-	 * associated with each event type.
+	 * According to SWIM, an event should be sent to all
+	 * members at least once - for that a TTL (time-to-live)
+	 * counter is maintained for each independent event type.
==================================================================

I dropped "It'd be silly ..." because I do not see a link to that
sentence. Sending old outdated member attribute value and sending
an attribute only once has nothing to do in common.

> +	 *
> +	 * When a member state changes, the TTL is reset to the
> +	 * cluster size. It is then decremented after each send.
> +	 * This guarantees that each member state change is sent
> +	 * to each SWIM member at least once. If a new event of
> +	 * the same type is generated before a round is finished,
> +	 * we update the current event object and reset the
> +	 * relevant TTL.

My update:

==================================================================
-	 * we update the current event object and reset the
-	 * relevant TTL.
+	 * the current event object is updated in place with reset
+	 * of the TTL.
==================================================================

I dropped 'we'.

> +	 *
> +	 * To conclude, TTL works in two ways: to see which
> +	 * specific member attribute needs dissemination and to
> +	 * track how many cluster members still need to learn
> +	 * about the change from us.

My update:

==================================================================
-	 * about the change from us.
+	 * about the change from this instance.
==================================================================

I dropped 'us'.

> +	 */
> +
> +	/**
> +	 * status_ttl is reset whenever a member status changes.
> +	 * We also reset it whenever any other attribute changes,
> +	 * to be able to easily track whether we need to send
> +	 * any events at all.
> +	 * Besides, a dead member can not be removed from the
> +	 * member table until its status TTL is 0, so as to allow
> +	 * other members learn its dead status.

My update:

==================================================================
 	/**
-	 * status_ttl is reset whenever a member status changes.
-	 * We also reset it whenever any other attribute changes,
-	 * to be able to easily track whether we need to send
-	 * any events at all.
-	 * Besides, a dead member can not be removed from the
-	 * member table until its status TTL is 0, so as to allow
-	 * other members learn its dead status.
+	 * General TTL reset each time when any visible member
+	 * attribute is updated. It is always bigger or equal than
+	 * any other TTLs. In addition it helps to keep a dead
+	 * member not dropped until the TTL gets zero so as to
+	 * allow other members to learn the dead status.
==================================================================

I dropped 'status_ttl' name from all the comments, because it is an
erroneous practice to duplicate compilable code into comments. If
in future status_ttl name is changed, we 100% will not update some
of the comments and will have the same that we deal in SQLite code
with.

Also, I dropped 'we'.

>  	 */
>  	int status_ttl;
>  	/**
> -	 * Events are put into a queue sorted by event occurrence
> -	 * time.
> +	 * All created events are put into a queue sorted by event time.
>  	 */
> -	struct rlist in_events_queue;
> +	struct rlist in_event_queue;
>  };
> @@ -708,7 +739,7 @@ swim_new_round(struct swim *swim)
>  /**
>   * Encode anti-entropy header and random members data as many as
>   * possible to the end of the packet.
> - * @retval Number of key-values added to the packet's root map.
> + * @retval Number of key-value pairs added to the packet's root map.

I've reverted that change, because

 - it has nothing in common with the patch. It is an old anti-entropy
   comment, not a new dissemination code.

 - we have exactly the same @retval comment on other swim_encode_...()
   functions. I think, that they should be consistent.

 - it pollutes git history.

>   */
>  static int
>  swim_encode_anti_entropy(struct swim *swim, struct swim_packet *packet)