From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 D41EE445324 for ; Wed, 8 Jul 2020 15:07:41 +0300 (MSK) Date: Wed, 8 Jul 2020 15:07:40 +0300 From: Sergey Bronnikov Message-ID: <20200708120740.GC58485@pony.bronevichok.ru> References: <012c8c196396cf963a0aa1f2d23814ff84b81cfb.1593723973.git.sergeyb@tarantool.org> <20200707121224.GA58485@pony.bronevichok.ru> <95ae952b-4062-9ebe-2af3-3ed58502fc1b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <95ae952b-4062-9ebe-2af3-3ed58502fc1b@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 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