Re: [tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sun, Mar 03, 2019 at 11:26:17PM +0300, Georgy Kirichenko wrote:
> Use apply_row for bot initial join and subscribe.

s/bot/both

> Refactoring: add memtx and vinyl engine static variable, get rid of
> engine_by_name at box.cc

Why do you need that? Looking at the next patch, I see no point at all
in such a refactoring.

> 
> Needed for: 2798
> ---
>  src/box/applier.cc |  2 +-
>  src/box/box.cc     | 51 ++++++++++++++++------------------------------
>  src/box/box.h      |  9 --------
>  3 files changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index fd98b733d..3222b041d 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -314,7 +314,7 @@ applier_join(struct applier *applier)
>  		coio_read_xrow(coio, ibuf, &row);
>  		applier->last_row_time = ev_monotonic_now(loop());
>  		if (iproto_type_is_dml(row.type)) {
> -			if (apply_initial_join_row(&row) != 0)
> +			if (apply_row(&row) != 0)
>  				diag_raise();
>  			if (++row_count % 100000 == 0)
>  				say_info("%.1fM rows received", row_count / 1e6);
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 22cd52b04..e62f2ea12 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -137,6 +137,15 @@ static struct fiber_pool tx_fiber_pool;
>   */
>  static struct cbus_endpoint tx_prio_endpoint;
>  
> +/**
> + * Memtx engine instance
> + */
> +static struct memtx_engine *memtx = NULL;
> +/**
> + * Vinyl engine instance
> + */
> +static struct vinyl_engine *vinyl = NULL;
> +
>  static int
>  box_check_writable(void)
>  {
> @@ -306,6 +315,15 @@ apply_row(struct xrow_header *row)
>  {
>  	struct request request;
>  	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
> +
> +	assert(memtx != NULL);
> +	if (memtx->state == MEMTX_INITIAL_RECOVERY) {
> +		/* Snapshot recovery or initial join */
> +		struct space *space = space_cache_find_xc(request.space_id);
> +		/* no access checks here - applier always works with admin privs */
> +		return space_apply_initial_join_row(space, &request);
> +	}
> +

Mixing two essentially different cases (initial join and subscribe) in
one function, using 'if' to switch between them, accessing memtx->state
here - all of this looks convoluted and confusing. Let's please let
apply_initial_join_row be.

>  	if (request.type == IPROTO_NOP) {
>  		if (process_nop(&request) != 0)
>  			return -1;
> @@ -349,16 +367,6 @@ wal_stream_create(struct wal_stream *ctx, size_t wal_max_rows)
>  	ctx->yield = (wal_max_rows >> 4)  + 1;
>  }
>  
> -int
> -apply_initial_join_row(struct xrow_header *row)
> -{
> -	struct request request;
> -	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
> -	struct space *space = space_cache_find_xc(request.space_id);
> -	/* no access checks here - applier always works with admin privs */
> -	return space_apply_initial_join_row(space, &request);
> -}
> -
>  /* {{{ configuration bindings */
>  
>  static void
> @@ -784,13 +792,9 @@ box_set_io_collect_interval(void)
>  void
>  box_set_snap_io_rate_limit(void)
>  {
> -	struct memtx_engine *memtx;
> -	memtx = (struct memtx_engine *)engine_by_name("memtx");

Getting rid of engine_by_name may be worthwhile, but it's definitely out
of the scope of your task. Besides, this looks incomplete, as we still
use engine_by_name in other places (lua/stat.c, lua/info.c). If you
think, we need to drop engine_by_name and access engines directly, then
please submit it in a separate patch with a proper justification. For
now, let's please drop this patch from this series.