* [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
* 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