From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 CF892469719 for ; Sat, 15 Feb 2020 02:50:06 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 15 Feb 2020 00:50:05 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 4/4] test: add tests for truncation and deletion List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , tarantool-patches@dev.tarantool.org Thanks for the patch! Please, put tests to the same commit as the patch. See 10 comments below. On 14/02/2020 20:39, Ilya Kosarev wrote: > Trying to perform space:truncate() and space:delete() while reaching > memtx_memory limit we could experience slab allocator failure. Now it > is solved through quota overuse tech. This commit introduces > corresponding tests. > > Part of #3807 > --- > src/box/memtx_space.c | 15 +++- > src/lib/core/errinj.h | 1 + > test/box/errinj.result | 1 + > test/engine/engine.cfg | 6 ++ > test/engine/low_memory.lua | 8 ++ > test/engine/stress_delete.result | 111 +++++++++++++++++++++++++++ > test/engine/stress_delete.test.lua | 55 +++++++++++++ > test/engine/stress_truncate.result | 103 +++++++++++++++++++++++++ > test/engine/stress_truncate.test.lua | 52 +++++++++++++ > 9 files changed, 349 insertions(+), 3 deletions(-) > create mode 100644 test/engine/low_memory.lua > create mode 100644 test/engine/stress_delete.result > create mode 100644 test/engine/stress_delete.test.lua > create mode 100644 test/engine/stress_truncate.result > create mode 100644 test/engine/stress_truncate.test.lua > > diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg > index f1e7de274..c8c6fb130 100644 > --- a/test/engine/engine.cfg > +++ b/test/engine/engine.cfg > @@ -5,6 +5,12 @@ > }, > "func_index.test.lua": { > "memtx": {"engine": "memtx"} > + }, > + "stress_delete.test.lua": { > + "memtx": {"engine": "memtx"} > + }, > + "stress_truncate.test.lua": { > + "memtx": {"engine": "memtx"} > } 1. This folder (engine/) is supposed to have only tests running on both engines. It is purpose of this folder. If you need something to run for one engine only, please, add it to a different folder. For you case the box/ folder is ok. > } > > diff --git a/test/engine/low_memory.lua b/test/engine/low_memory.lua > new file mode 100644 > index 000000000..46fd26d1b > --- /dev/null > +++ b/test/engine/low_memory.lua > @@ -0,0 +1,8 @@ > +#!/usr/bin/env tarantool > +os = require('os') > +box.cfg({ > + listen = os.getenv("LISTEN"), > + memtx_memory = 32 * 1024 * 1024, > +}) > + > +require('console').listen(os.getenv('ADMIN')) > diff --git a/test/engine/stress_delete.result b/test/engine/stress_delete.result > new file mode 100644 > index 000000000..bf92c780b > --- /dev/null > +++ b/test/engine/stress_delete.result 2. Since this is a test for a bug, and I would say quite a big test, it should follow the convention and have the corresponding name pattern: 'gh-####-short-description.test.lua'. 3. Please, don't split test of that bug into separate files. You just duplicated lots of the code. 4. The test uses errinj, but is not added to release_disabled. Please, add. > @@ -0,0 +1,111 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... 5. When a test is related to a ticket (which is nearly always), you need to write a comment saying ticket number and its description. See examples in other test files. > + > + > +test_run:cmd("create server master with script='engine/low_memory.lua'") > + | --- > + | - true > + | ... > +test_run:cmd('start server master') > + | --- > + | - true > + | ... > +test_run:cmd("switch master") > + | --- > + | - true > + | ... > + > + > +test_run:cmd("setopt delimiter ';'") > + | --- > + | - true > + | ... > +function create_space(name) > + local space = box.schema.create_space(name) > + space:format({ > + { name = "id", type = "unsigned" }, > + { name = "val", type = "str" } > + }) > + space:create_index('primary', { parts = { 'id' } }) > + return space > +end; > + | --- > + | ... > + > +function insert(space, i) > + space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) }) > +end; 6. Why does it matter whether data is random? IMO, on the contrary, it reduces reproducibility. > + | --- > + | ... > + > +function fill_space(space, start) > + local _, err = nil > + local i = start > + while err == nil do _, err = pcall(insert, space, i) i = i + 1 end > +end; > + | --- > + | ... > + > +function stress_deletion(i, spaces) > + local res, space = pcall(create_space, 'test' .. tostring(i)) > + if res then spaces[i] = space return end > + fill_space(box.space.test, box.space.test:len()) > + for _, s in pairs(spaces) do fill_space(s, s:len()) end > + box.space.test:delete(box.space.test:len() - 1) > +end; > + | --- > + | ... > +test_run:cmd("setopt delimiter ''"); > + | --- > + | - true > + | ... > + > + > +_ = create_space('test') > + | --- > + | ... > +for i = 0, 27000 do insert(box.space.test, i) end 7. Why exactly 27k? 8. Seems like drop of this space 'test' does not change anything. I dropped it and the test passed. > + | --- > + | ... > + > +spaces = {} > + | --- > + | ... > +counter = 0 > + | --- > + | ... > +status = true > + | --- > + | ... > +res = nil > + | --- > + | ... > +errinj = box.error.injection > + | --- > + | ... > +errinj.set('ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE', true) 9. I dropped that error injection and nothing changed in the output. What do you need it for? > + | --- > + | - ok > + | ... > +while counter < 1400 do status, res = pcall(stress_deletion, counter, spaces) counter = counter + 1 end 10. Why 1400? > + | --- > + | ... > +status > + | --- > + | - true > + | ... > +res > + | --- > + | - null > + | ... I have more comments, but I will keep them until I understand what is happening in these tests.