[Tarantool-patches] [PATCH] relay: yield explicitly every N sent rows

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 12.02.2021 12:25, Serge Petrenko via Tarantool-patches wrote:
> While sending a WAL, relay only yields in `coio_write_xrow`, once it
> sees the socket isn't ready for writes.
> It may happen that the socket is always ready for a long period of time,
> and relay doesn't yield at all while recovering a whole .xlog file. This
> may take well more than a minute.
> During this period of time, relay doesn't read replica's ACKs due to
> relay reader fiber not being scheduled, and once the reader is finally
> live it times out immediately, causing the replica to reconnect.
> 
> The problem is amplified by the fact that replica waits for
> replication_timeout to pass prior to reconnecting, which lets master
> pile up even more ready WALs, and effectively making it impossible for
> the replica to sync.

I couldn't understand this part. Why is it bad? Yeah, replica waits,
but replica is applier, on another instance. How is it related? And
relay_reader does not send anything. So why is it bad?

Couldn't the problem be fixed by reading all the non-consumed data after
reading WAL?

The current solution also looks fine. Maybe even better because it
becomes consistent with local recovery. However I still want to
understand this part about replica.

> To fix the problem let's yield explicitly in relay_send_row every
> WAL_ROWS_PER_YIELD rows. The same is already done in local recovery, and
> serves the same purpose: to not block the event loop for too long.
> 
> Closes #5762
> ---
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index df04f8198..afc57dfbc 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -836,11 +836,20 @@ relay_send(struct relay *relay, struct xrow_header *packet)
>  {
>  	ERROR_INJECT_YIELD(ERRINJ_RELAY_SEND_DELAY);
>  
> +	static uint64_t row_cnt = 0;

Relays are in threads. So this variable either should be thread-local,
or be in struct relay. Otherwise you get non-atomic updates which may
lead to some increments disappearing.

Given that thread-local variable access is not free, I would go for
having it in struct relay, but up to you.

>  	packet->sync = relay->sync;
>  	relay->last_row_time = ev_monotonic_now(loop());
>  	coio_write_xrow(&relay->io, packet);
>  	fiber_gc();
>  
> +	/*
> +	 * It may happen that the socket is always ready for write, so yield
> +	 * explicitly every now and then to not block the event loop.
> +	 */
> +	row_cnt++;
> +	if (row_cnt % WAL_ROWS_PER_YIELD == 0) {
> +		fiber_sleep(0);
> +	}

Maybe better drop {} as the if's body is just one line.