Re: [Tarantool-patches] [PATCH 2/4] replication: add advanced tests for sync replication

[Sergey Bronnikov <sergeyb@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@