From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 81A13445320 for ; Thu, 6 Aug 2020 11:29:45 +0300 (MSK) Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1k3bHU-0004GE-Mx for tarantool-patches@dev.tarantool.org; Thu, 06 Aug 2020 11:29:45 +0300 Received: by mail-lj1-f172.google.com with SMTP id z14so13198453ljm.1 for ; Thu, 06 Aug 2020 01:29:44 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yaroslav Dynnikov Date: Thu, 6 Aug 2020 11:29:33 +0300 Message-ID: Content-Type: multipart/alternative; boundary="0000000000005a2fd405ac314a22" 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 Cc: Yaroslav Dynnikov , tml --0000000000005a2fd405ac314a22 Content-Type: text/plain; charset="UTF-8" Hi! Nice patchset, LGTM. Best regards Yaroslav Dynnikov On Thu, 6 Aug 2020 at 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', > -- > 2.21.1 (Apple Git-122.3) > > --0000000000005a2fd405ac314a22 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi! Nice patchset, LGTM.

Best regards
Yaroslav Dynnikov
<= /div>


On Thu, 6 Aug 2020 at 01:15, Vladis= lav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
Reload evolution is a subsystem for storag= e 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
---
=C2=A0test/reload_evolution/storage.result=C2=A0 =C2=A0| 6 +++++-
=C2=A0test/reload_evolution/storage.test.lua | 6 +++++-
=C2=A0vshard/storage/reload_evolution.lua=C2=A0 =C2=A0 | 2 +-
=C2=A03 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/s= torage.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'] =3D nil
=C2=A0vshard.storage =3D require("vshard.storage")
=C2=A0---
=C2=A0...
-test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolut= ion: upgraded to') ~=3D 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 cal= led 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_evolut= ion: upgraded to') =3D=3D nil
=C2=A0---
=C2=A0- true
=C2=A0...
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})
=C2=A0package.path =3D original_package_path
=C2=A0package.loaded['vshard.storage'] =3D nil
=C2=A0vshard.storage =3D require("vshard.storage")
-test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolut= ion: upgraded to') ~=3D 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 cal= led 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_evolut= ion: upgraded to') =3D=3D nil
=C2=A0vshard.storage.internal.reload_version
=C2=A0-- Make sure storage operates well.
=C2=A0vshard.storage.bucket_force_drop(2000)
diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_ev= olution.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)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0log.error(err_msg)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error(err_msg)
=C2=A0 =C2=A0 =C2=A0end
-=C2=A0 =C2=A0 for i =3D start_version, #migrations=C2=A0 do
+=C2=A0 =C2=A0 for i =3D start_version + 1, #migrations do
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0local ok, err =3D pcall(migrations[i], M)=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ok then
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0log.info('vshard.storage.relo= ad_evolution: upgraded to %d version',
--
2.21.1 (Apple Git-122.3)

--0000000000005a2fd405ac314a22--