[Tarantool-patches] [PATCH v2] recovery: make it yield when positioning in a WAL
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 12 23:36:17 MSK 2021
Hi! Thanks for the patch!
See 4 comments below.
On 12.05.2021 13:29, Serge Petrenko wrote:
> We had various places in box.cc and relay.cc which counted processed
> rows and yielded every now and then. These yields didn't cover cases,
> when recovery has to position inside a long WAL file:
>
> For example, when tarantool exits without leaving an empty WAL file
> which'll be used to recover instance vclock on restart. In this case
> the instance freezes while processing the last availabe WAL in order
1. availabe -> available.
> diff --git a/changelogs/unreleased/gh-5979-recovery-ligs.md b/changelogs/unreleased/gh-5979-recovery-ligs.md
> new file mode 100644
> index 000000000..86abfd66a
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5979-recovery-ligs.md
> @@ -0,0 +1,11 @@
> +# bugfix/core
> +
> +* Now tarantool yields when scanning `.xlog` files for the latest applied vclock
> + and when finding the right place in `.xlog`s to start recovering. This means
> + that the instance is responsive right after `box.cfg` call even when an empty
> + `.xlog` was not created on previous exit.
> + Also this prevents relay from timing out when a freshly subscribed replica
> + needs rows from the end of a relatively long (hundreds of MBs) `.xlog`
> + (gh-5979).
2. Maybe an empty line here? Could simplify reading a bit.
> +* The counter in `x.yM rows processed` log messages does not reset on each new
> + recovered `xlog` anymore.
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 59925962d..8a7b8593d 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -610,15 +597,28 @@ end_error:
> diag_raise();
> }
>
> +static struct wal_stream wal_stream;
> +
> +/**
> + * Plan a yield in recovery stream. Wal stream will execute it as soon as it's
> + * ready.
> + */
> +static void
> +wal_stream_schedule_yield(void)
3. Since now you have an object owning the callback, you
could pass it as a first argument, like 'this' in C++,
and like apply_row() does. Then you wouldn't need the
global wal_stream.
> +{
> + wal_stream.has_yield = true;
> + wal_stream_try_yield(&wal_stream);
> +}
> diff --git a/src/box/xstream.h b/src/box/xstream.h
> index d29ff4213..d27de09a3 100644
> --- a/src/box/xstream.h
> +++ b/src/box/xstream.h
> @@ -41,16 +41,40 @@ extern "C" {
> struct xrow_header;
> struct xstream;
>
> +/**
> + * A type for a callback invoked by recovery after some batch of rows is
> + * processed. Is used mostly to unblock the event loop every now and then.
> + */
> +typedef void (*schedule_yield_f)(void);
4. How about moving closer to the consistency and calling it
xstream_yield_f/xstream_schedule_yield_f? So it has xstream_ prefix
like xstream_write_f does.
More information about the Tarantool-patches
mailing list