From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 5 Mar 2019 13:26:34 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row Message-ID: <20190305102634.64o6tyns5eo5af6o@esperanza> References: <1c0402648b0ce2f9a74bdd9cf4b95fb26e0fd297.1551644303.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1c0402648b0ce2f9a74bdd9cf4b95fb26e0fd297.1551644303.git.georgy@tarantool.org> To: Georgy Kirichenko Cc: tarantool-patches@freelists.org List-ID: 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.