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

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

13.02.2021 00:48, 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?

Thanks for the review!

I shouldn't have included this paragraph to the explanation probably.
I tried to explain how this bug leads to replica not being able to sync
with master when master's under load.

I reworded the commit message a bit, hope it's more clear now.

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

Relay does read every ack received while feeding a WAL, but it reads the 
acks only
after finishing reading WAL, so all the reads time-out.

>
> 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.

Thanks for noticing! Let it be in relay then.
Diff:

================================================
diff --git a/src/box/relay.cc b/src/box/relay.cc
index 1d8edf116..6d9269e1d 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -117,6 +117,11 @@ struct relay {
          * is passed by the replica on subscribe.
          */
         uint32_t id_filter;
+       /**
+        * How many rows has this relay sent to the replica. Used to 
yield once
+        * in a while when reading a WAL to unblock the event loop.
+        */
+       size_t row_cnt;
         /**
          * Local vclock at the moment of subscribe, used to check
          * dataset on the other side and send missing data rows if any.
@@ -218,6 +223,7 @@ relay_start(struct relay *relay, int fd, uint64_t sync,
         coio_create(&relay->io, fd);
         relay->sync = sync;
         relay->state = RELAY_FOLLOW;
+       relay->row_cnt = 0;
         relay->last_row_time = ev_monotonic_now(loop());
  }

@@ -836,7 +842,6 @@ relay_send(struct relay *relay, struct xrow_header 
*packet)
  {
         ERROR_INJECT_YIELD(ERRINJ_RELAY_SEND_DELAY);

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

         struct errinj *inj = errinj(ERRINJ_RELAY_TIMEOUT, ERRINJ_DOUBLE);



>
>>   	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.
Already fixed in reply to Cyrill.

-- 
Serge Petrenko