From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 B33C54696C3 for ; Fri, 10 Apr 2020 21:24:11 +0300 (MSK) Date: Fri, 10 Apr 2020 18:24:10 +0000 From: Nikita Pettik Message-ID: <20200410182410.GA10478@tarantool.org> References: <3595ddfb-4635-61b8-97ae-105f9994087d@tarantool.org> <20200410154050.GD9428@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200410154050.GD9428@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 10 Apr 15:40, Nikita Pettik wrote: > On 10 Apr 17:13, Vladislav Shpilevoy wrote: > > > + | --- > > > + | ... > > > +dump() > > > + | --- > > > + | ... > > > +-- 1 range, 2 runs > > > > 2. Can you add an assertion, that it is really 1 range and 2 runs? > > To be sure that the test won't become useless in future if suddenly > > something will change there. > > Ok, will add. > > > > + > > > +compact() > > > + | --- > > > + | ... > > > +-- 1 range, 1 run after compaction > > > + > > > +dump() > > > + | --- > > > + | ... > > > +-- 1 range, 2 runs > > > + > > > +errinj = box.error.injection > > > + | --- > > > + | ... > > > +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0) > > > + | --- > > > + | - ok > > > + | ... > > > +-- Should finish successfully despite vy_stmt_alloc() fail. > > > +-- > > > +compact() > > > + | --- > > > + | ... > > > +-- 1 range, 1 run > > > +s.index.pk:stat().range_count > > > + | --- > > > + | - 1 > > > + | ... > > > +s.index.pk:stat().run_count > > > > 3. I would add an assertion/print, that ERRINJ_VY_MAX_TUPLE_SIZE > > (or whatever will be used after the review) is changed back to > > off, and then change it to off again anyway. Because if the test > > won't hit the errinj because of something in future, the errinj > > will be left set, and we won't notice that here. Also it will > > make all next vinyl tests running in the same job fail in very > > surprising ways. > > > > The same for the next commit. > > Ok, will fix. > Diff: diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua index 53a050c9b..693a4ff22 100644 --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua @@ -29,13 +29,17 @@ test_run:cmd("setopt delimiter ''"); -- dump(true) dump() +assert(s.index.pk:stat().range_count == 1) +assert(s.index.pk:stat().run_count == 2) -- 1 range, 2 runs compact() --- 1 range, 1 run after compaction +assert(s.index.pk:stat().range_count == 1) +assert(s.index.pk:stat().run_count == 1) dump() --- 1 range, 2 runs +assert(s.index.pk:stat().range_count == 1) +assert(s.index.pk:stat().run_count == 2) errinj = box.error.injection errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0) @@ -43,7 +47,9 @@ errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0) -- compact() -- 1 range, 1 run -s.index.pk:stat().range_count -s.index.pk:stat().run_count +assert(s.index.pk:stat().range_count == 1) +assert(s.index.pk:stat().run_count == 1) +assert(errinj.get('ERRINJ_VY_MAX_TUPLE_SIZE') == -1) +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', -1) s:drop()