From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 B0881445320 for ; Thu, 6 Aug 2020 09:40:04 +0300 (MSK) From: Oleg Babin References: Message-ID: <6179c32d-0970-db1b-09a4-178d8ab102b0@tarantool.org> Date: Thu, 6 Aug 2020 09:40:02 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 vshard 1/2] storage: fix reload applying migration twice List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org Hi! Thanks for your patch. LGTM. On 06/08/2020 01:15, Vladislav Shpilevoy wrote: > Reload evolution is a subsystem for storage hot code reload. When > code is reloaded, it applies migration scripts one by one. But > there was a bug - the last migration script was applied even when > code version didn't really change. > > The bug didn't affect anything, since so far the reload evolution > was not used for anything and had just one dummy migration script > doing nothing. > > Now a first migration script is going to be added, so the bug is > fixed before that. > > Needed for #237 > --- > test/reload_evolution/storage.result | 6 +++++- > test/reload_evolution/storage.test.lua | 6 +++++- > vshard/storage/reload_evolution.lua | 2 +- > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result > index ebf4fdc..dde0a1f 100644 > --- a/test/reload_evolution/storage.result > +++ b/test/reload_evolution/storage.result > @@ -86,7 +86,11 @@ package.loaded['vshard.storage'] = nil > vshard.storage = require("vshard.storage") > --- > ... > -test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil > +-- Should be nil. Because there was a bug that reload always reapplied the last > +-- migration callback. When it was fixed, the last callback wasn't called twice. > +-- But since the callback was only one, now nothing is called, and nothing is > +-- logged. > +test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil > --- > - true > ... > diff --git a/test/reload_evolution/storage.test.lua b/test/reload_evolution/storage.test.lua > index 56c1693..7858112 100644 > --- a/test/reload_evolution/storage.test.lua > +++ b/test/reload_evolution/storage.test.lua > @@ -35,7 +35,11 @@ box.space.test:insert({42, bucket_id_to_move}) > package.path = original_package_path > package.loaded['vshard.storage'] = nil > vshard.storage = require("vshard.storage") > -test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil > +-- Should be nil. Because there was a bug that reload always reapplied the last > +-- migration callback. When it was fixed, the last callback wasn't called twice. > +-- But since the callback was only one, now nothing is called, and nothing is > +-- logged. > +test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil > vshard.storage.internal.reload_version > -- Make sure storage operates well. > vshard.storage.bucket_force_drop(2000) > diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua > index 5d09b11..1abc9e2 100644 > --- a/vshard/storage/reload_evolution.lua > +++ b/vshard/storage/reload_evolution.lua > @@ -33,7 +33,7 @@ local function upgrade(M) > log.error(err_msg) > error(err_msg) > end > - for i = start_version, #migrations do > + for i = start_version + 1, #migrations do > local ok, err = pcall(migrations[i], M) > if ok then > log.info('vshard.storage.reload_evolution: upgraded to %d version',