Re: [Tarantool-patches] [PATCH v2] recovery: make it yield when positioning in a WAL

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

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.