Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Georgy Kirichenko <georgy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row
Date: Tue, 5 Mar 2019 13:26:34 +0300	[thread overview]
Message-ID: <20190305102634.64o6tyns5eo5af6o@esperanza> (raw)
In-Reply-To: <1c0402648b0ce2f9a74bdd9cf4b95fb26e0fd297.1551644303.git.georgy@tarantool.org>

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.

  parent reply	other threads:[~2019-03-05 10:26 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-03 20:26 [tarantool-patches] [PATCH 0/3] Transaction boundaries for applier Georgy Kirichenko
2019-03-03 20:26 ` [tarantool-patches] [PATCH 1/3] Applier gets rid of a xstream Georgy Kirichenko
2019-03-05  8:52   ` [tarantool-patches] " Konstantin Osipov
2019-03-05 10:19   ` [tarantool-patches] " Vladimir Davydov
2019-03-03 20:26 ` [tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row Georgy Kirichenko
2019-03-05  9:06   ` [tarantool-patches] " Konstantin Osipov
2019-03-05 10:26   ` Vladimir Davydov [this message]
2019-03-03 20:26 ` [tarantool-patches] [PATCH 3/3] Transaction support for applier Georgy Kirichenko
2019-03-05  9:11   ` [tarantool-patches] " Konstantin Osipov
2019-03-05 10:28     ` Vladimir Davydov
     [not found]       ` <20190305112333.GA30697@chai>
2019-03-05 11:27         ` Vladimir Davydov
2019-03-05  9:13   ` Konstantin Osipov
2019-03-05  9:25   ` Konstantin Osipov
2019-03-05  9:28   ` Konstantin Osipov
2019-03-05 11:59   ` [tarantool-patches] " Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190305102634.64o6tyns5eo5af6o@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=georgy@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox