From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 13 Jun 2019 19:21:48 +0300 From: Vladimir Davydov Subject: Re: [PATCH] recovery: remove yields from index build and format check Message-ID: <20190613162148.zrzxirap4vqbc6ra@esperanza> References: <20190613153038.70080-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190613153038.70080-1-sergepetrenko@tarantool.org> To: Serge Petrenko Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 13, 2019 at 06:30:38PM +0300, Serge Petrenko wrote: > Iproto already listens for requests during recovery, so yielding at this > point of time allows such early requests, which arrived during recovery, > be processed while data is in unfinished state. This caused box/net.box > test failures, and is potentially harmful. > Besides, there is no need to yield during recovery. > > Closes #4273 > --- > https://github.com/tarantool/tarantool/issues/4273 > https://github.com/tarantool/tarantool/tree/sp/gh-4273-dont-yield-on-recovery > > src/box/memtx_space.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Pushed to master with a few nitpicks (see below). > > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 9f232803e..3e607e0e6 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -47,6 +47,9 @@ > * Yield every 1K tuples while building a new index or checking > * a space format. In debug mode yield more often for testing > * purposes. > + * Yields do not happen during recovery. At this point of time > + * iproto aready accepts requests, and yielding would allow them > + * to be proccessed while data is not fully recovered. Added a newline between paragraphs - text looks more readable that way. > */ > #ifdef NDEBUG > enum { MEMTX_DDL_YIELD_LOOPS = 1000 }; > @@ -884,6 +887,7 @@ memtx_space_check_format(struct space *space, struct tuple_format *format) > if (it == NULL) > return -1; > > + struct memtx_engine *memtx = (struct memtx_engine *)space->engine; > struct memtx_ddl_state state; > state.format = format; > state.cmp_def = pk->def->key_def; > @@ -905,7 +909,7 @@ memtx_space_check_format(struct space *space, struct tuple_format *format) > rc = tuple_validate(format, tuple); > if (rc != 0) > break; > - if (++count % MEMTX_DDL_YIELD_LOOPS == 0) { > + if (++count % MEMTX_DDL_YIELD_LOOPS == 0 && memtx->state == MEMTX_OK) { The line's too long. Split. > state.cursor = tuple; > tuple_ref(state.cursor); > fiber_sleep(0); > @@ -1033,6 +1037,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index, > if (it == NULL) > return -1; > > + struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine; > struct memtx_ddl_state state; > state.index = new_index; > state.format = new_format; > @@ -1079,7 +1084,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index, > */ > if (new_index->def->iid == 0) > tuple_ref(tuple); > - if (++count % MEMTX_DDL_YIELD_LOOPS == 0) { > + if (++count % MEMTX_DDL_YIELD_LOOPS == 0 && memtx->state == MEMTX_OK) { Ditto.