From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 B1AC2445320 for ; Thu, 9 Jul 2020 12:39:02 +0300 (MSK) Date: Thu, 9 Jul 2020 12:39:01 +0300 From: Sergey Bronnikov Message-ID: <20200709093901.GA26360@pony.bronevichok.ru> References: <012c8c196396cf963a0aa1f2d23814ff84b81cfb.1593723973.git.sergeyb@tarantool.org> <20200707121224.GA58485@pony.bronevichok.ru> <95ae952b-4062-9ebe-2af3-3ed58502fc1b@tarantool.org> <20200708120740.GC58485@pony.bronevichok.ru> <809fdb7b-5aac-3dc5-2d10-af6f8b5903c6@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <809fdb7b-5aac-3dc5-2d10-af6f8b5903c6@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/4] replication: add advanced tests for sync replication 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 00:13 Thu 09 Jul , Vladislav Shpilevoy wrote: > On 08/07/2020 14:07, Sergey Bronnikov wrote: > > On 22:57 Tue 07 Jul , Vladislav Shpilevoy wrote: > >>>>> test_run:switch('default') > >>>>> box.cfg{replication_synchro_quorum=NUM_INSTANCES, replication_synchro_timeout=0.1} > >>>>> _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > >>>>> _ = box.space.sync:create_index('pk') > >>>>> -- Testcase body. > >>>>> box.space.sync:insert{1} > >>>>> box.space.sync:insert{2} > >>>>> box.space.sync:insert{3} > >>>>> box.space.sync:select{} -- 1, 2, 3 > >>>>> test_run:switch('replica') > >>>>> box.space.sync:select{} -- 1, 2, 3 > >>>>> -- Testcase cleanup. > >>>>> test_run:switch('default') > >>>>> box.space.sync:drop() > >>>>> > >>>>> -- Synchro timeout is not bigger than replication_synchro_timeout value. > >>>>> -- Testcase setup. > >>>>> test_run:switch('default') > >>>>> box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=orig_synchro_timeout} > >>>>> _ = box.schema.space.create('sync', {is_sync=true, engine=engine}) > >>>>> _ = box.space.sync:create_index('pk') > >>>>> -- Testcase body. > >>>>> start = os.time() > >>>>> box.space.sync:insert{1} > >>>>> (os.time() - start) == box.cfg.replication_synchro_timeout -- true > >>>> > >>>> 7. Очень плохая идея. Если процесс подвиснет тут ненадолго, то эта проверка > >>>> упадет. Не должно быть тестов, которые полагаются на то, что процесс будет > >>>> выполняться стабильно. > >>> > >>> Ты предлагаешь не проверять или есть более надежные способы проверки, > >>> что таймаут именно такой величины, каким его выставили? > >> > >> Если тебе надо проверить, что таймаут провалился, то надо проверять, > >> что прошедшее время >= timeout, но точно не == timeout. Второе очень > >> ненадежно. > >> > > > > Мне не нравится эта проверка, потому что тест должен проверять, что "timeout > > not bigger than replication_synchro_timeout value". > > Сделал так: > > > > box.space.sync:insert{1} > > -(os.time() - start) == box.cfg.replication_synchro_timeout -- true > > +-- We assume that the process may freeze and the timeout will be slightly > > +-- larger than the set value. > > +POSSIBLE_ERROR = 2 > > +(os.time() - start) < box.cfg.replication_synchro_timeout + POSSIBLE_ERROR -- true > > -- Testcase cleanup. > > Во-первых, твой тест как раз проверяет падение таймаута. Потому что ты > пытаешься писать с BROKEN_QUORUM. Так что проверка походу неверна. > > Во-вторых, даже если бы кворум был, то хак с добавкой нескольких секунд > тоже не дает гарантий, а значит тест становится flaky. Не надо так делать, > пожалуйста. Сделал проверку, что таймаут не меньше указанного значения: --- Synchro timeout is not bigger than replication_synchro_timeout value. +-- Synchro timeout is not less than replication_synchro_timeout value. -- Testcase setup. test_run:switch('default') box.cfg{replication_synchro_quorum=BROKEN_QUORUM, replication_synchro_timeout=orig_synchro_timeout} @@ -51,10 +52,7 @@ _ = box.space.sync:create_index('pk') -- Testcase body. start = os.time() box.space.sync:insert{1} --- We assume that the process may freeze and the timeout will be slightly --- larger than the set value. -POSSIBLE_ERROR = 2 -(os.time() - start) < box.cfg.replication_synchro_timeout + POSSIBLE_ERROR -- true +(os.time() - start) >= box.cfg.replication_synchro_timeout -- true -- Testcase cleanup. > >>>>> test_run:cmd("setopt delimiter ';'") > >>>>> _ = fiber.create(function() > >>>>> box.space.sync:insert{2} > >>>>> end); > >>>>> test_run:cmd("setopt delimiter ''"); > >>>>> -- Disable synchronous mode. > >>>>> disable_sync_mode() > >>>>> -- Space is in async mode now. > >>>>> box.space.sync:insert{3} -- async operation must wait sync one > >>>>> box.error.injection.set('ERRINJ_SYNC_TIMEOUT', false) > >>>>> box.space.sync:select{} -- 1 > >>>>> test_run:cmd('switch replica') > >>>>> box.space.sync:select{} -- 1 > >>>>> -- Testcase cleanup. > >>>>> test_run:switch('default') > >>>>> box.space.sync:drop() > >>>>> > >>>>> -- Warn user when setting `replication_synchro_quorum` to a value > >>>>> -- greater than number of instances in a cluster, see gh-5122. > >>>>> box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- warning > >>>> > >>>> 13. Этот тест походу вообще ничего не проверяет. Варнинг сейчас не пишется, > >>>> и тест проходит. > >>> > >>> Обычный процесс такой: если тест падает, то, пока есть открытая > >>> проблема, добавляют XFAIL и при изменении поведения XFAIL меняется на > >>> XPASS, чтобы убрать XFAIL. У нас нет такого механизма, поэтому добавил > >>> тест на будущее и когда варнинг добавят, то тест сломается и result файл > >>> обновят. Мне кажется, что это вполне себе ок. > >> > >> Это было бы ок, если бы коммент говорил, что тест пока невалидный. Но что > >> еще важнее - тест все равно пройдет даже когда добавится ворнинг. Потому > >> что он пойдет в лог, и в выводе теста его не будет. Так что тест пройдет, > >> хоть и не должен. > > > > У нас автоматические тесты с бинарным статусом PASS или FAIL и человек > > обычно смотрит результат выполнения тестов, а не комментарии в > > исходнике. Поэтому это не сильно меняет дело. Но я обновил комментарий: > > > > -- greater than number of instances in a cluster, see gh-5122. > > -box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- warning > > +box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- expected warning, to be add in gh-5122 > > Ты видимо не прочитал, что я написал(. Печать ворнинга не изменит вообще > ничего. Ворнинги - это логи, они не идут в дифф. Они будут в лог файле, > а не в выводе теста. И этот тест все равно пройдет. Да, я тебя не так понял. Тогда можно так, но grep_log() нужно будет потом раскомментировать: --- greater than number of instances in a cluster, see gh-5122. -box.cfg{replication_synchro_quorum=BROKEN_QUORUM} -- expected warning, to be add in gh-5122 +-- should be greater than number of instances in a cluster, see gh-5122. +box.cfg{replication_synchro_quorum=BROKEN_QUORUM} +-- expected warning, to be add in gh-5122 +--while test_run:grep_log('default', 'warning: .*') == nil do fiber.sleep(0.01) end -- sergeyb@