From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 472B3297E6 for ; Sun, 24 Mar 2019 03:19:42 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JFsofv56AIlA for ; Sun, 24 Mar 2019 03:19:42 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 749A829798 for ; Sun, 24 Mar 2019 03:19:41 -0400 (EDT) Date: Sun, 24 Mar 2019 10:19:34 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v2 1/2] test: tunned timeouts and added statuses checks Message-ID: <20190324071934.6pfpaxs6s7bl3xoy@tkn_work_nb> References: <6726cdd338a2c24005e72ceec9a6badc072bcdbd.1553148177.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <6726cdd338a2c24005e72ceec9a6badc072bcdbd.1553148177.git.avtikhon@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "Alexander V. Tikhonov" Cc: tarantool-patches@freelists.org In short: the patch too large to review each individual change (say, timeouts or wait_cond's). Also there are changes I doubt about (fast_replica usage for a single instance creation and fast_replica changes). I see, it was the big effort, but I don't see gains. Comments are inlined below. WBR, Alexander Turenko. On Thu, Mar 21, 2019 at 09:03:05AM +0300, Alexander V. Tikhonov wrote: > Tunned timeouts to the common value. Added for status checks the > the wait_cond loops with diagnostic, changed naming of the tests > replicas, separated replicas between subtests in tests, changed > hardcoded replicas creation/deletion to the standalone routine. Tunned -> tuned. In the commit header too. > Tunned timeouts to the common value. How one can review it within this large patch? Need to be factored out to allow one to look into them. > Added for status checks the the wait_cond loops with diagnostic. Ok. > Changed naming of the tests replicas. Is it about file names (and test-run instance names)? I think that it would be better to create one instance.lua or replica.lua (if possible) and use symlinks. It is not good to duplicate code. > Separated replicas between subtests in tests. Is it part of the previous item? > Changed hardcoded replicas creation/deletion to the standalone > routine. It does not become less 'hardcoded' when becomes wrapped. AFAIU, you wrap 'create server', 'start server' and other test-run's commands into 'fast_replica' even when we start just one server. So complexity of tests and these wrappes (fast_replica.lua) both were increased. I don't understand gains of this change. > @@ -485,26 +481,26 @@ fiber = require('fiber') > while box.info.replication[1].upstream.status ~= 'follow' do fiber.sleep(0.0001) end > --- > ... > -box.info.replication[1].upstream.status -- follow > +test_run:wait_cond(function() return box.info.replication[1].upstream.status == 'follow' end) or box.info.replication[1].upstream.status > --- > -- follow > +- true 'while' loop above do the same thing. It should be removed I guess. > ... > for i = 0, 15 do fiber.sleep(0.01) if box.info.replication[1].upstream.status ~= 'follow' then break end end > --- > ... > -box.info.replication[1].upstream.status -- follow > +test_run:wait_cond(function() return box.info.replication[1].upstream.status == 'follow' end) or box.info.replication[1].upstream.status Same here. > --- a/test/replication/lua/fast_replica.lua > +++ b/test/replication/lua/fast_replica.lua > @@ -1,18 +1,23 @@ > > -function join(inspector, n) > - local path = os.getenv('TARANTOOL_SRC_DIR') > +function create(inspector, name, replica) > + replica = replica or 'replica' So now name of an instance file can be arbirtary, but different from names in a test. It will make investigation of a test fail more complex. > function unregister(inspector, id) > - box.space._cluster:delete{id} > + id = id or 2 I understood that you try to simplify 'one instance' case, but this default looks really counter-intuitive. I don't think that it worth to use 'fast_replica' for that case as I wrote above. > +function hibernate(inspector, name, id) > + return stop(inspector, name, id) and cleanup(inspector, name, id) > end I have to remember now what is create, start, stop and clean. Enough terms I think. > -function delete(inspector, id) > - inspector:cmd('stop server replica'..tostring(id - 1)) > - inspector:cmd('delete server replica'..tostring(id - 1)) > +function prune(inspector, name, id) > + return unregister(inspector, id) and drop(inspector, name, id) > +end Same here. > diff --git a/test/replication/suite.ini b/test/replication/suite.ini > index 6e9e3edd0..aa79f68f3 100644 > --- a/test/replication/suite.ini > +++ b/test/replication/suite.ini > @@ -7,5 +7,6 @@ release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.l > config = suite.cfg > lua_libs = lua/fast_replica.lua lua/rlimit.lua > use_unix_sockets = True > +use_unix_sockets_iproto = True It is not mentioned in the commit message. And should be factored out into its own patch.