[Tarantool-patches] [PATCH] relay: yield explicitly every N sent rows
Serge Petrenko
sergepetrenko at tarantool.org
Mon Feb 15 11:40:19 MSK 2021
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
More information about the Tarantool-patches
mailing list