Re: [Tarantool-patches] [PATCH v4 13/12] replication: send accumulated Raft messages after relay start

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Good job on the patch!

See 4 comments below.

> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index 7be33ee31..9fdd02bc1 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -160,6 +160,16 @@ struct relay {
>           * anonymous replica, for example.
>           */
>          bool is_raft_enabled;
> +        /** Is set to true by the first Raft broadcast which comes while

1. Should be a new line after /**.

> +         * the relay is not yet ready to dispatch Raft messages.
> +         */
> +        bool has_pending_broadcast;
> +        /**
> +         * A Raft broadcast which should be pushed once relay notifies
> +         * tx it needs Raft updates. Otherwise this message would be
> +         * lost until some new Raft event happens.
> +         */
> +        struct raft_request pending_broadcast;

2. I wouldn't call them 'broadcasts'. Relay sends a single message to
the remote node, not to all the nodes. This is a broadcast on the raft
level. On relay level it is just a single message to one node.

>      } tx;
>  };
> @@ -635,14 +649,28 @@ tx_set_is_raft_enabled(struct cmsg *base)
>      struct relay_is_raft_enabled_msg *msg =
>          (struct relay_is_raft_enabled_msg *)base;
>      msg->relay->tx.is_raft_enabled = msg->value;
> +    if (msg->relay->tx.has_pending_broadcast) {
> +        msg->has_pending_broadcast = true;
> +        msg->req = msg->relay->tx.pending_broadcast;

3. Since you will deliver the broadcast now, it is not pending
anymore. Hence there must be msg->relay->tx.has_pending_broadcast = false
in the end.

> +    }
>  }
> @@ -964,12 +1008,15 @@ void
>  relay_push_raft(struct relay *relay, const struct raft_request *req)
>  {
>      /*
> -     * Raft updates don't stack. They are thrown away if can't be pushed
> -     * now. This is fine, as long as relay's live much longer that the
> -     * timeouts in Raft are set.
> +     * Remember the latest Raft update. It might be a notification that
> +     * this node is a leader. If sometime later we find out this node needs
> +     * Raft updates, we need to send is_leader notification.
>       */
> -    if (!relay->tx.is_raft_enabled)
> +    if (!relay->tx.is_raft_enabled) {
> +        relay->tx.has_pending_broadcast = true;
> +        relay->tx.pending_broadcast = *req;

4. Vclock memory does not belong to the request. This is why below we copy
it into the message's memory. You might need to do the same here.

>          return;
> +    }
>      /*
>       * XXX: the message should be preallocated. It should
>       * work like Kharon in IProto. Relay should have 2 raft

We could also fix it like described in this XXX, could we?