[tarantool-patches] [PATCH 2/3] Merge apply row and apply_initial_join_row
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Mar 5 13:26:34 MSK 2019
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.
More information about the Tarantool-patches
mailing list