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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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.

> >>> -- Testcase cleanup.
> >>> test_run:switch('default')
> >>> box.space.sync:drop()
> >>>
> >>> -- replication_synchro_quorum
> >>> test_run:switch('default')
> >>> INT_MIN = -2147483648
> >>> INT_MAX = 2147483648
> >>> box.cfg{replication_synchro_quorum=INT_MAX} -- error
> >>> box.cfg.replication_synchro_quorum -- old value
> >>> box.cfg{replication_synchro_quorum=INT_MIN} -- error
> >>> box.cfg.replication_synchro_quorum -- old value
> >>
> >> 8. Это тоже явно не advanced тесты. Это самые базовые проверки.
> > 
> > Я изначально делал тесты в отдельном файле, чтобы проще было изменять
> > это в общей ветке, без мержей, ребейзов и прочих вещей. Тесты назвались
> > advanced, потомы что должны были покрывать высокоуровневые требования из
> > RFC. Я могу перенести эти тесты в qsync_basic, если возражений по сути
> > тестов нет.
> 
> Да, лучше бы в basic.

Перенес.

> >>> -- Testcase setup.
> >>> 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.begin() box.space.sync:insert({1}) box.commit()
> >>> box.begin() box.space.sync:insert({2}) box.commit()
> >>> -- Testcase cleanup.
> >>> box.space.sync:drop()
> >>>
> >>> -- [RFC, summary] switch sync replicas into async ones, expected success and
> >>> -- data consistency on a leader and replicas.
> >>
> >> 10. Это пожалуй пока единственный тест, который тут можно было бы оставить.
> >> То есть 'advanced'. Но коммент неверен - нет никаких синхронных реплик.
> >> Есть синхронные транзакции. Которые определяются синхронными спейсами.
> > 
> > RFC: "ability to switch async replicas into sync ones and vice versa"
> >                      ^^^^^^^^^^^^^^^^^^^
> > В тесте поправлю комментарий. Еще, как я понял, у тебя были возражения
> > по поводу того, как делаем выключение синхронной репликации, чтобы она
> > стала асинхронной. Или запись в системный спейс это ок?
> 
> Пока ок. Потом будет интерфейс нормальный. Сейчас у спейса ничего кроме
> формата поменять нельзя нормально. Надо эту проблему решать в общем
> случае.

завел тикет на это https://github.com/tarantool/tarantool/issues/5155

> >>> 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