From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id C25CD46970E
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 15 Jan 2020 20:36:19 +0300 (MSK)
Date: Wed, 15 Jan 2020 20:36:19 +0300
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200115173619.GF7851@tarantool.org>
References: <20191202161128.20890-1-bokuno@picodata.io>
 <20191205112752.GA11015@tarantool.org>
 <1578908907.647593657@f120.i.mail.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1578908907.647593657@f120.i.mail.ru>
Subject: Re: [Tarantool-patches] [PATCH] vinyl: prohibit
 vinyl_level_size_ratio less than two
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Maxim Kulis <bokuno@picodata.io>
Cc: tarantool-patches@dev.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