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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>>> 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. Второе очень
ненадежно.

>>> -- 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"
>                      ^^^^^^^^^^^^^^^^^^^
> В тесте поправлю комментарий. Еще, как я понял, у тебя были возражения
> по поводу того, как делаем выключение синхронной репликации, чтобы она
> стала асинхронной. Или запись в системный спейс это ок?

Пока ок. Потом будет интерфейс нормальный. Сейчас у спейса ничего кроме
формата поменять нельзя нормально. Надо эту проблему решать в общем
случае.

>>> 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 файл
> обновят. Мне кажется, что это вполне себе ок.

Это было бы ок, если бы коммент говорил, что тест пока невалидный. Но что
еще важнее - тест все равно пройдет даже когда добавится ворнинг. Потому
что он пойдет в лог, и в выводе теста его не будет. Так что тест пройдет,
хоть и не должен.