* [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two @ 2019-12-02 16:11 Maksim Kulis 2019-12-05 11:27 ` Nikita Pettik 0 siblings, 1 reply; 6+ messages in thread From: Maksim Kulis @ 2019-12-02 16:11 UTC (permalink / raw) To: tarantool-patches Change the minimum possible value of the variable vinyl_run_size_ratio to 2. Closes #3346 --- src/box/box.cc | 4 ++-- test/box-tap/cfg.test.lua | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/box/box.cc b/src/box/box.cc index be2390335..e426fdab8 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -589,9 +589,9 @@ box_check_vinyl_options(void) tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level", "must be greater than 0"); } - if (run_size_ratio <= 1) { + if (run_size_ratio < 2) { tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio", - "must be greater than 1"); + "must be greater than or uqual to 2"); } if (bloom_fpr <= 0 || bloom_fpr > 1) { tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr", diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua index d529447bb..b68c6f17b 100755 --- a/test/box-tap/cfg.test.lua +++ b/test/box-tap/cfg.test.lua @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0) invalid('vinyl_write_threads', 1) invalid('vinyl_page_size', 0) invalid('vinyl_run_count_per_level', 0) -invalid('vinyl_run_size_ratio', 1) +invalid('vinyl_run_size_ratio', 1.9) invalid('vinyl_bloom_fpr', 0) invalid('vinyl_bloom_fpr', 1.1) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two 2019-12-02 16:11 [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two Maksim Kulis @ 2019-12-05 11:27 ` Nikita Pettik [not found] ` <1578908907.647593657@f120.i.mail.ru> 0 siblings, 1 reply; 6+ messages in thread From: Nikita Pettik @ 2019-12-05 11:27 UTC (permalink / raw) To: Maksim Kulis; +Cc: tarantool-patches On 02 Dec 19:11, Maksim Kulis wrote: > Change the minimum possible value of the variable vinyl_run_size_ratio to 2. > > Closes #3346 > --- > src/box/box.cc | 4 ++-- > test/box-tap/cfg.test.lua | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) Hello. Thanks for the patch. Firsly, please update check of run_size_ratio during index options decode (see index_opts_decode()). I would also update assert() in vy_range_update_compaction_priority(): let's make it check that run_size_ratio always >= 2. Finally, I would add brief explanation to the commit message why run_size_ratio less than 2 is meaningless (for those who are not familiar with vinyl inner organization). > diff --git a/src/box/box.cc b/src/box/box.cc > index be2390335..e426fdab8 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -589,9 +589,9 @@ box_check_vinyl_options(void) > tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level", > "must be greater than 0"); > } > - if (run_size_ratio <= 1) { > + if (run_size_ratio < 2) { > tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio", > - "must be greater than 1"); > + "must be greater than or uqual to 2"); > } > if (bloom_fpr <= 0 || bloom_fpr > 1) { > tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr", > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua > index d529447bb..b68c6f17b 100755 > --- a/test/box-tap/cfg.test.lua > +++ b/test/box-tap/cfg.test.lua > @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0) > invalid('vinyl_write_threads', 1) > invalid('vinyl_page_size', 0) > invalid('vinyl_run_count_per_level', 0) > -invalid('vinyl_run_size_ratio', 1) > +invalid('vinyl_run_size_ratio', 1.9) > invalid('vinyl_bloom_fpr', 0) > invalid('vinyl_bloom_fpr', 1.1) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1578908907.647593657@f120.i.mail.ru>]
* Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two [not found] ` <1578908907.647593657@f120.i.mail.ru> @ 2020-01-15 17:36 ` Nikita Pettik 2020-01-24 11:04 ` Maxim Kulis 0 siblings, 1 reply; 6+ messages in thread From: Nikita Pettik @ 2020-01-15 17:36 UTC (permalink / raw) To: Maxim Kulis; +Cc: tarantool-patches On 13 Jan 12:48, Maxim Kulis wrote: > Hi! > Thanks for your review. > Hi, Are you going to update your patch according to the review notes? Review process is described here: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review > >Четверг, 5 декабря 2019, 14:27 +03:00 от Nikita Pettik <korablev@tarantool.org>: > > > >On 02 Dec 19:11, Maksim Kulis wrote: > >> Change the minimum possible value of the variable vinyl_run_size_ratio to 2. > >> > >> Closes #3346 > >> --- > >> src/box/box.cc | 4 ++-- > >> test/box-tap/cfg.test.lua | 2 +- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > > > >Hello. Thanks for the patch. > > > >Firsly, please update check of run_size_ratio during index options > >decode (see index_opts_decode()). > > > >I would also update assert() in vy_range_update_compaction_priority(): > >let's make it check that run_size_ratio always >= 2. > > > >Finally, I would add brief explanation to the commit message why > >run_size_ratio less than 2 is meaningless (for those who are not > >familiar with vinyl inner organization). > > > >> diff --git a/src/box/box.cc b/src/box/box.cc > >> index be2390335..e426fdab8 100644 > >> --- a/src/box/box.cc > >> +++ b/src/box/box.cc > >> @@ -589,9 +589,9 @@ box_check_vinyl_options(void) > >> tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level", > >> "must be greater than 0"); > >> } > >> - if (run_size_ratio <= 1) { > >> + if (run_size_ratio < 2) { > >> tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio", > >> - "must be greater than 1"); > >> + "must be greater than or uqual to 2"); > >> } > >> if (bloom_fpr <= 0 || bloom_fpr > 1) { > >> tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr", > >> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua > >> index d529447bb..b68c6f17b 100755 > >> --- a/test/box-tap/cfg.test.lua > >> +++ b/test/box-tap/cfg.test.lua > >> @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0) > >> invalid('vinyl_write_threads', 1) > >> invalid('vinyl_page_size', 0) > >> invalid('vinyl_run_count_per_level', 0) > >> -invalid('vinyl_run_size_ratio', 1) > >> +invalid('vinyl_run_size_ratio', 1.9) > >> invalid('vinyl_bloom_fpr', 0) > >> invalid('vinyl_bloom_fpr', 1.1) > >> > >> -- > >> 2.17.1 > >> > > > -- > Maxim Kulis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two 2020-01-15 17:36 ` Nikita Pettik @ 2020-01-24 11:04 ` Maxim Kulis 2020-01-31 0:24 ` Nikita Pettik 0 siblings, 1 reply; 6+ messages in thread From: Maxim Kulis @ 2020-01-24 11:04 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 6343 bytes --] Hi! Yes, here is the new patch. Change the minimum possible value of the variable vinyl_run_size_ratio to 2. Closes #3346 issue: https://github.com/tarantool/tarantool/issues/3346 --- src/box/alter.cc | 4 ++-- src/box/box.cc | 4 ++-- src/box/vy_range.c | 2 +- test/box-tap/cfg.test.lua | 2 +- test/vinyl/ddl.result | 5 +++-- test/vinyl/ddl.test.lua | 2 +- 6 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index bef25b605..123e94994 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -227,10 +227,10 @@ index_opts_decode(struct index_opts *opts, const char *map, "run_count_per_level must be greater than 0"); return -1; } - if (opts->run_size_ratio <= 1) { + if (opts->run_size_ratio < 2) { diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, BOX_INDEX_FIELD_OPTS, - "run_size_ratio must be greater than 1"); + "run_size_ratio must be greater than or uqual to 2"); return -1; } if (opts->bloom_fpr <= 0 || opts->bloom_fpr > 1) { diff --git a/src/box/box.cc b/src/box/box.cc index b119c927b..0f26e28cd 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -589,9 +589,9 @@ box_check_vinyl_options(void) tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level", "must be greater than 0"); } - if (run_size_ratio <= 1) { + if (run_size_ratio < 2) { tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio", - "must be greater than 1"); + "must be greater than or uqual to 2"); } if (bloom_fpr <= 0 || bloom_fpr > 1) { tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr", diff --git a/src/box/vy_range.c b/src/box/vy_range.c index 4ff852112..8d6bfd30b 100644 --- a/src/box/vy_range.c +++ b/src/box/vy_range.c @@ -289,7 +289,7 @@ vy_range_update_compaction_priority(struct vy_range *range, const struct index_opts *opts) { assert(opts->run_count_per_level > 0); - assert(opts->run_size_ratio > 1); + assert(opts->run_size_ratio >= 2); range->compaction_priority = 0; vy_disk_stmt_counter_reset(&range->compaction_queue); diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua index d529447bb..b68c6f17b 100755 --- a/test/box-tap/cfg.test.lua +++ b/test/box-tap/cfg.test.lua @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0) invalid('vinyl_write_threads', 1) invalid('vinyl_page_size', 0) invalid('vinyl_run_count_per_level', 0) -invalid('vinyl_run_size_ratio', 1) +invalid('vinyl_run_size_ratio', 1.9) invalid('vinyl_bloom_fpr', 0) invalid('vinyl_bloom_fpr', 1.1) diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result index 9c453a007..a97e415be 100644 --- a/test/vinyl/ddl.result +++ b/test/vinyl/ddl.result @@ -23,9 +23,10 @@ space:create_index('pk', {run_count_per_level = 0}) - error: 'Wrong index options (field 4): run_count_per_level must be greater than 0' ... -space:create_index('pk', {run_size_ratio = 1}) +space:create_index('pk', {run_size_ratio = 1.9}) --- -- error: 'Wrong index options (field 4): run_size_ratio must be greater than 1' +- error: 'Wrong index options (field 4): run_size_ratio must be greater than or uqual + to 2' ... space:create_index('pk', {bloom_fpr = 0}) --- diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua index 010ec6d79..35e37a84f 100644 --- a/test/vinyl/ddl.test.lua +++ b/test/vinyl/ddl.test.lua @@ -6,7 +6,7 @@ space = box.schema.space.create('test', {engine = 'vinyl' }) space:create_index('pk', {page_size = 0}) space:create_index('pk', {page_size = 8192, range_size = 4096}) space:create_index('pk', {run_count_per_level = 0}) -space:create_index('pk', {run_size_ratio = 1}) +space:create_index('pk', {run_size_ratio = 1.9}) space:create_index('pk', {bloom_fpr = 0}) space:create_index('pk', {bloom_fpr = 1.1}) space:drop() -- 2.17.1 >Среда, 15 января 2020, 20:36 +03:00 от Nikita Pettik < korablev@tarantool.org >: > >On 13 Jan 12:48, Maxim Kulis wrote: >> Hi! >> Thanks for your review. >> > >Hi, > >Are you going to update your patch according to the review notes? >Review process is described here: >https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/#how-to-submit-a-patch-for-review > >> >Четверг, 5 декабря 2019, 14:27 +03:00 от Nikita Pettik < korablev@tarantool.org >: >> > >> >On 02 Dec 19:11, Maksim Kulis wrote: >> >> Change the minimum possible value of the variable vinyl_run_size_ratio to 2. >> >> >> >> Closes #3346 >> >> --- >> >> src/box/box.cc | 4 ++-- >> >> test/box-tap/cfg.test.lua | 2 +- >> >> 2 files changed, 3 insertions(+), 3 deletions(-) >> > >> >Hello. Thanks for the patch. >> > >> >Firsly, please update check of run_size_ratio during index options >> >decode (see index_opts_decode()). >> > >> >I would also update assert() in vy_range_update_compaction_priority(): >> >let's make it check that run_size_ratio always >= 2. >> > >> >Finally, I would add brief explanation to the commit message why >> >run_size_ratio less than 2 is meaningless (for those who are not >> >familiar with vinyl inner organization). >> > >> >> diff --git a/src/box/box.cc b/src/box/box.cc >> >> index be2390335..e426fdab8 100644 >> >> --- a/src/box/box.cc >> >> +++ b/src/box/box.cc >> >> @@ -589,9 +589,9 @@ box_check_vinyl_options(void) >> >> tnt_raise(ClientError, ER_CFG, "vinyl_run_count_per_level", >> >> "must be greater than 0"); >> >> } >> >> - if (run_size_ratio <= 1) { >> >> + if (run_size_ratio < 2) { >> >> tnt_raise(ClientError, ER_CFG, "vinyl_run_size_ratio", >> >> - "must be greater than 1"); >> >> + "must be greater than or uqual to 2"); >> >> } >> >> if (bloom_fpr <= 0 || bloom_fpr > 1) { >> >> tnt_raise(ClientError, ER_CFG, "vinyl_bloom_fpr", >> >> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua >> >> index d529447bb..b68c6f17b 100755 >> >> --- a/test/box-tap/cfg.test.lua >> >> +++ b/test/box-tap/cfg.test.lua >> >> @@ -46,7 +46,7 @@ invalid('vinyl_read_threads', 0) >> >> invalid('vinyl_write_threads', 1) >> >> invalid('vinyl_page_size', 0) >> >> invalid('vinyl_run_count_per_level', 0) >> >> -invalid('vinyl_run_size_ratio', 1) >> >> +invalid('vinyl_run_size_ratio', 1.9) >> >> invalid('vinyl_bloom_fpr', 0) >> >> invalid('vinyl_bloom_fpr', 1.1) >> >> >> >> -- >> >> 2.17.1 >> >> >> >> >> -- >> Maxim Kulis -- Maxim Kulis [-- Attachment #2: Type: text/html, Size: 9029 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two 2020-01-24 11:04 ` Maxim Kulis @ 2020-01-31 0:24 ` Nikita Pettik 2020-02-04 0:27 ` Maxim Kulis 0 siblings, 1 reply; 6+ messages in thread From: Nikita Pettik @ 2020-01-31 0:24 UTC (permalink / raw) To: Maxim Kulis; +Cc: tarantool-patches On 24 Jan 14:04, Maxim Kulis wrote: > > > Hi! > Yes, here is the new patch. > > Change the minimum possible value of the variable vinyl_run_size_ratio to 2. > > Closes #3346 > > issue: https://github.com/tarantool/tarantool/issues/3346 > --- Put a link to the issue below '---' delimiter next time. > >> >Finally, I would add brief explanation to the commit message why > >> >run_size_ratio less than 2 is meaningless (for those who are not > >> >familiar with vinyl inner organization). Please, add a brief explanation to the commit message and then patch will be OK to push. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two 2020-01-31 0:24 ` Nikita Pettik @ 2020-02-04 0:27 ` Maxim Kulis 0 siblings, 0 replies; 6+ messages in thread From: Maxim Kulis @ 2020-02-04 0:27 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 1721 bytes --] Hi! > Put a link to the issue below '---' delimiter next time. Ok! > Please, add a brief explanation to the commit message and then patch will > be OK to push. Thanks. When vinyl_run_size_ratio less then 2, LSM-tree could work incorrect. For example: Suppose the L0 size is 100Mb, the ratio of run sizes at each LSM-tree level is 1.5 and there can be no more than 2 runs per level (the variable vinyl_run_count_per_level). After the first 3 dumps, the disk will contain 3 runs of 100 Mb at L1. Let the dump contain many delete operations, so the runs will be compacted into a single 200 Mb run. It will be moved to L2. The next 3 dumps will produce 300 Mb run and it will be moved to L3, because there could be only 150-225 Mb files at L2. the new file is lower than the old one, what contradicts the basic idea of the tree LSM-tree. Similarly with the vinyl_run_count_per_level = 1. branch: https://github.com/maksimkulis/tarantool/tree/maksimkulis/bh-3346-fix-run-size-ratio >Пятница, 31 января 2020, 3:24 +03:00 от Nikita Pettik <korablev@tarantool.org>: > >On 24 Jan 14:04, Maxim Kulis wrote: >> >> >> Hi! >> Yes, here is the new patch. >> >> Change the minimum possible value of the variable vinyl_run_size_ratio to 2. >> >> Closes #3346 >> >> issue: https://github.com/tarantool/tarantool/issues/3346 >> --- > >Put a link to the issue below '---' delimiter next time. > >> >> >Finally, I would add brief explanation to the commit message why >> >> >run_size_ratio less than 2 is meaningless (for those who are not >> >> >familiar with vinyl inner organization). > >Please, add a brief explanation to the commit message and then patch will >be OK to push. Thanks. > -- Maxim Kulis [-- Attachment #2: Type: text/html, Size: 2673 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-04 0:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 16:11 [Tarantool-patches] [PATCH] vinyl: prohibit vinyl_level_size_ratio less than two Maksim Kulis
2019-12-05 11:27 ` Nikita Pettik
[not found] ` <1578908907.647593657@f120.i.mail.ru>
2020-01-15 17:36 ` Nikita Pettik
2020-01-24 11:04 ` Maxim Kulis
2020-01-31 0:24 ` Nikita Pettik
2020-02-04 0:27 ` Maxim Kulis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox