From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 37F39445320 for ; Thu, 9 Jul 2020 01:13:40 +0300 (MSK) References: <012c8c196396cf963a0aa1f2d23814ff84b81cfb.1593723973.git.sergeyb@tarantool.org> <20200707121224.GA58485@pony.bronevichok.ru> <95ae952b-4062-9ebe-2af3-3ed58502fc1b@tarantool.org> <20200708120740.GC58485@pony.bronevichok.ru> From: Vladislav Shpilevoy Message-ID: <809fdb7b-5aac-3dc5-2d10-af6f8b5903c6@tarantool.org> Date: Thu, 9 Jul 2020 00:13:37 +0200 MIME-Version: 1.0 In-Reply-To: <20200708120740.GC58485@pony.bronevichok.ru> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org 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. Не надо так делать, пожалуйста. >>>>> 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 Ты видимо не прочитал, что я написал(. Печать ворнинга не изменит вообще ничего. Ворнинги - это логи, они не идут в дифф. Они будут в лог файле, а не в выводе теста. И этот тест все равно пройдет.