From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 D9C6F445320 for ; Tue, 7 Jul 2020 23:57:17 +0300 (MSK) References: <012c8c196396cf963a0aa1f2d23814ff84b81cfb.1593723973.git.sergeyb@tarantool.org> <20200707121224.GA58485@pony.bronevichok.ru> From: Vladislav Shpilevoy Message-ID: <95ae952b-4062-9ebe-2af3-3ed58502fc1b@tarantool.org> Date: Tue, 7 Jul 2020 22:57:15 +0200 MIME-Version: 1.0 In-Reply-To: <20200707121224.GA58485@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 >>> 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 файл > обновят. Мне кажется, что это вполне себе ок. Это было бы ок, если бы коммент говорил, что тест пока невалидный. Но что еще важнее - тест все равно пройдет даже когда добавится ворнинг. Потому что он пойдет в лог, и в выводе теста его не будет. Так что тест пройдет, хоть и не должен.