[tarantool-patches] Re: [PATCH 2/2] wal: drop rows_per_wal option
Alexander Turenko
alexander.turenko at tarantool.org
Wed Oct 9 19:05:33 MSK 2019
The patch itself LGTM.
The motivation why it is needed is not quite clear. Sergey, Kirill,
please, see the first cited discussion below.
WBR, Alexander Turenko.
----
> >> Rows_per_wal option was deprecated because it can be covered by
> >> wal_max_size. In order not to complicate WAL code with that
> >> option's support this commit drops it completely.
> >
> > It is just one 'if' subcondition in the code and one field for storing
> > yields frequency. (And the field can be replaced with a static variable
> > w/o removing the option.) I don't see how this commit simplifies the
> > code.
>
> AFAIU this complicates not only the present code, but also future code,
> according to Kostja's comment in the issue description:
>
> "We will have trouble with automatic size management if we
> continue using wal_max_rows"
>
> But honestly I don't really care that much about this option. I just
> decided to fix the unassigned issue, marked as breaking change, as
> soon as possible, to break less users. So I can't provide any serious
> evidence how rows_per_wal was going to complicate auto size management.
CCed Sergos and Kirill Yu.
> > The option was convenient for tests at least. Now additional
> > calculations are necessary to dig into cases that verifies xlog files
> > count.
>
> If that is needed for testing only, then it should not be exposed to
> public API.
Good point.
>
> >
> > However I don't think that those cons and pros are important, so I don't
> > want to block the patch. The patch itself looks good except minor
> > comments below.
> >
> >>
> >> In some tests the option was used to create several small xlog
> >> files. Now the same is done via wal_max_size. Where it was
> >> needed, number of rows per wal is estimated as wal_max_size / 50.
> >> Because struct xrow_header size ~= 50 not counting paddings and
> >> body.
> >>
> >> Closes #3762
> >> ---
> >> src/box/box.cc | 30 +-------
> >> src/box/lua/load_cfg.lua | 5 --
> >> src/box/wal.c | 14 +---
> >> src/box/wal.h | 13 +++-
> >> test/app-tap/init_script.result | 39 +++++-----
> >> test/app-tap/snapshot.test.lua | 2 +-
> >> test/app/app.lua | 2 +-
> >> test/app/fiber.result | 6 +-
> >> test/app/fiber.test.lua | 4 +-
> >> test/box-py/box.lua | 2 +-
> >> test/box-tap/cfg.test.lua | 3 +-
> >> test/box/admin.result | 2 -
> >> test/box/cfg.result | 4 -
> >> test/box/configuration.result | 107 ---------------------------
> >> test/box/proxy.lua | 2 +-
> >> test/box/tiny.lua | 1 -
> >> test/engine/box.lua | 2 +-
> >> test/engine_long/box.lua | 1 -
> >> test/long_run-py/box.lua | 1 -
> >> test/vinyl/vinyl.lua | 1 -
> >> test/xlog-py/box.lua | 1 -
> >> test/xlog/checkpoint_daemon.result | 11 ++-
> >> test/xlog/checkpoint_daemon.test.lua | 9 ++-
> >> test/xlog/errinj.result | 7 +-
> >> test/xlog/errinj.test.lua | 5 +-
> >> test/xlog/panic.lua | 1 -
> >> test/xlog/upgrade/fill.lua | 2 +-
> >> test/xlog/xlog.lua | 2 +-
> >> 28 files changed, 72 insertions(+), 207 deletions(-)
> >> delete mode 100644 test/box/configuration.result
> >
> >> static void
> >> -wal_stream_create(struct wal_stream *ctx, size_t wal_max_rows)
> >> +wal_stream_create(struct wal_stream *ctx)
> >> {
> >> xstream_create(&ctx->base, apply_wal_row);
> >> ctx->rows = 0;
> >> - /**
> >> - * Make the yield logic covered by the functional test
> >> - * suite, which has a small setting for rows_per_wal.
> >> - * Each yield can take up to 1ms if there are no events,
> >> - * so we can't afford many of them during recovery.
> >> - */
> >> - ctx->yield = (wal_max_rows >> 4) + 1;
> >
> > Yield frequency is not more depend on wal file size / rows count, so we
> > we'll not test this scenario anymore. Look:
> >
> > https://coveralls.io/builds/26111576/source?filename=src/box/box.cc#L345
> > https://coveralls.io/builds/25591832/source?filename=src/box/box.cc#L343
> >
> > Don't sure whether it is really important, but it seems that someone
> > paid special attention to this and even leave the comment about.
> >
> > Can we, say, store 'rows per yield' in a static variable with 32000 as
> > the default value and allow tests to set it with errinj?
>
> There are too many tests which had small rows_per_wal setting. I don't
> think it is a good idea to make them all errinj dependent. That would
> require to disable them in release build, and to move to separate files.
> Instead, I added a new test:
Okay.
>
> ===========================================================================
>
> diff --git a/test/engine/recover_snapshot_wal.result b/test/engine/recover_snapshot_wal.result
> index d2667559d..6b7b05362 100644
> --- a/test/engine/recover_snapshot_wal.result
> +++ b/test/engine/recover_snapshot_wal.result
> @@ -23,9 +23,9 @@ box.snapshot()
> ---
> - ok
> ...
> -space:insert({1001})
> +space:insert({33001})
> ---
> -- [1001]
> +- [33001]
> ...
> test_run:cmd("restart server default")
> space = box.space['test']
> @@ -37,7 +37,7 @@ index = space.index['primary']
> index:select({}, {iterator = box.index.ALL})
> ---
> - - [0]
> - - [1001]
> + - [33001]
> ...
> for key = 1, 351 do space:insert({key}) end
> ---
> @@ -46,7 +46,18 @@ box.snapshot()
> ---
> - ok
> ...
> -for key = 352, 1000 do space:insert({key}) end
> +-- Insert so many tuples, that recovery would need to yield
> +-- periodically to allow other fibers do something. At the moment
> +-- of writing this the yield period was 32k tuples.
> +box.begin() \
> +for key = 352, 33000 do \
> + space:insert({key}) \
> + if key % 10000 == 0 then \
> + box.commit() \
> + box.begin() \
> + end \
> +end \
> +box.commit()
> ---
> ...
> test_run:cmd("restart server default")
> @@ -56,1010 +67,24 @@ space = box.space['test']
> index = space.index['primary']
> ---
> ...
> -index:select({}, {iterator = box.index.ALL})
> +i = 0
> ---
> -- - [0]
> - - [1]
> ... -- lots of deletions
> - - [999]
> - - [1000]
> - - [1001]
> +...
> +err = nil
> +---
> +...
> +for _, t in space:pairs() do \
> + if t[1] ~= i then \
> + err = {i, t} \
> + break \
> + end \
> + i = i + 1 \
> +end
> ---
> ...
> +i
> +---
> +- 33002
> +...
> +err
> +---
> +- null
> ...
> space:drop()
> ---
> diff --git a/test/engine/recover_snapshot_wal.test.lua b/test/engine/recover_snapshot_wal.test.lua
> index 1f133dd72..ff286bc4f 100644
> --- a/test/engine/recover_snapshot_wal.test.lua
> +++ b/test/engine/recover_snapshot_wal.test.lua
> @@ -10,7 +10,7 @@ index = space:create_index('primary')
>
> space:insert({0})
> box.snapshot()
> -space:insert({1001})
> +space:insert({33001})
>
> test_run:cmd("restart server default")
>
> @@ -20,13 +20,33 @@ index:select({}, {iterator = box.index.ALL})
>
> for key = 1, 351 do space:insert({key}) end
> box.snapshot()
> -for key = 352, 1000 do space:insert({key}) end
> +-- Insert so many tuples, that recovery would need to yield
> +-- periodically to allow other fibers do something. At the moment
> +-- of writing this the yield period was 32k tuples.
> +box.begin() \
> +for key = 352, 33000 do \
> + space:insert({key}) \
> + if key % 10000 == 0 then \
> + box.commit() \
> + box.begin() \
> + end \
> +end \
> +box.commit()
>
> test_run:cmd("restart server default")
>
> space = box.space['test']
> index = space.index['primary']
> -index:select({}, {iterator = box.index.ALL})
> +i = 0
> +err = nil
> +for _, t in space:pairs() do \
> + if t[1] ~= i then \
> + err = {i, t} \
> + break \
> + end \
> + i = i + 1 \
> +end
> +i
> +err
>
> space:drop()
> test_run:cmd("restart server default with cleanup=1")
>
> ===========================================================================
>
> But I don't think it was necessary. According to the comment (and
> to the patch introduced it), this signal-handling-sleep(0) was never
> properly tested. And even though this line was covered by our tests,
> its purpose was not tested.
I saw an issue when a yield leads to segfault:
https://github.com/tarantool/tarantool/issues/4110
It is good to have a regression test: at least it verifies that we'll
continue reading an xlog file after yield.
> >> diff --git a/test/xlog/errinj.test.lua b/test/xlog/errinj.test.lua
> >> index de0e2e7e8..3d72dc4e4 100644
> >> --- a/test/xlog/errinj.test.lua
> >> +++ b/test/xlog/errinj.test.lua
> >> @@ -30,12 +30,13 @@ _ = test:create_index('primary')
> >>
> >> box.schema.user.grant('guest', 'write', 'space', 'test')
> >>
> >> -for i=1, box.cfg.rows_per_wal do test:insert{i, 'test'} end
> >> +row_count_per_wal = box.cfg.wal_max_size / 50 + 10
> >> +for i=1, row_count_per_wal do test:insert{i, 'test'} end
> >
> > Is +10 here because tuples are a bit larger than in other tests?
> >
>
> No. Rather because in other tests it is too +10, and here it wasn't.
> Size of one wal record I estimated as >= 50 (size of xrow_header +
> msgpack headers + body size), so perhaps in theory / 50 was already
> enough. +10 is just in case, to ensure that row_count_per_wal 100%
> will create at least one new xlog file.
I misread this, because other tests do not add 10 rows to the variable,
but add them in a loop range:
| row_count_per_wal = box.cfg.wal_max_size / 50
| for i = 1, row_count_per_wal + 10 do <...> end
Anyway, it is ok.
More information about the Tarantool-patches
mailing list