From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 ED69943E888 for ; Tue, 24 Mar 2020 18:21:44 +0300 (MSK) Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1jGlNA-0008Bg-Bw for tarantool-patches@dev.tarantool.org; Tue, 24 Mar 2020 18:21:44 +0300 Received: by mail-lf1-f41.google.com with SMTP id j15so13560427lfk.6 for ; Tue, 24 Mar 2020 08:21:44 -0700 (PDT) MIME-Version: 1.0 References: <364ea64d7bf6beef8cea83ff2fbb432ba967cd80.1584817081.git.v.shpilevoy@tarantool.org> In-Reply-To: <364ea64d7bf6beef8cea83ff2fbb432ba967cd80.1584817081.git.v.shpilevoy@tarantool.org> From: Yaroslav Dynnikov Date: Tue, 24 Mar 2020 18:21:32 +0300 Message-ID: Content-Type: multipart/alternative; boundary="0000000000002eee7f05a19b4f83" Subject: Re: [Tarantool-patches] [PATCH vshard 1/2] storage: introduce upgrade strategy List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: yaroslav.dynnikov@tarantool.org, tarantool-patches@dev.tarantool.org --0000000000002eee7f05a19b4f83 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Vlad. I've run my cartridge upgrade tests on your branch (6e50e26c), and it's ok on tarantool 2.2, but fails for 1.10: ``` replica | ApplyConfigError: Space _schema does not support multi-statement transactions replica | stack traceback: replica | ...cartridge/.rocks/share/tarantool/vshard/storage/init.lua:419: in function 'schema_upgrade' replica | ...cartridge/.rocks/share/tarantool/vshard/storage/init.lua:2336: in function 'cfg' ``` Here are results from Gitlab CI: https://gitlab.com/tarantool/cartridge/pipelines/129256300 And here is one more remark about the patch itself. On Sat, 21 Mar 2020 at 21:59, Vladislav Shpilevoy wrote: > > local function this_is_master() > @@ -2169,8 +2276,12 @@ local function storage_cfg(cfg, this_replica_uuid, > is_reload) > error(err) > end > log.info("Box has been configured") > - local uri =3D luri.parse(this_replica.uri) > - box.once("vshard:storage:1", storage_schema_v1, uri.login, > uri.password) > + end > + > + local uri =3D luri.parse(this_replica.uri) > + schema_upgrade(is_master, uri.login, uri.password) > + > + if not is_reload then > It seems like this `if/else` statement isn't necessary. The `else` branch is enough for both cases. Even it's not hot reload it would result in `local old =3D nil; box.space._bucket:on_replace(new_trigger, nil)` which is essentially the same > box.space._bucket:on_replace(bucket_generation_increment) > else > local old =3D box.space._bucket:on_replace()[1] > @@ -2469,6 +2580,8 @@ M.rlist =3D { > add_tail =3D rlist_add_tail, > remove =3D rlist_remove, > } > +M.schema_latest_version =3D schema_latest_version > +M.schema_current_version =3D schema_current_version > > return { > sync =3D sync, > -- > 2.21.1 (Apple Git-122.3) > > --=20 =D0=A1 =D1=83=D0=B2=D0=B0=D0=B6=D0=B5=D0=BD=D0=B8=D0=B5=D0=BC. =D0=94=D1=8B=D0=BD=D0=BD=D0=B8=D0=BA=D0=BE=D0=B2 =D0=AF=D1=80=D0=BE=D1=81= =D0=BB=D0=B0=D0=B2. --0000000000002eee7f05a19b4f83 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi, Vlad.

I've run my ca= rtridge upgrade tests on your branch (6e50e26c), and it's ok on taranto= ol 2.2, but fails for 1.10:
```
replica | ApplyConfigError: Space _schema does n= ot support multi-statement transactions
replica | stack traceback:
<= /code>
re= plica | ...cart= ridge/.rocks/share/tarantool/vshard/storage/init.lua:419: in function '= schema_upgrade'
 replica | ...cartridge/.rock= s/share/tarantool/vshard/storage/init.lua:2336: in function 'cfg'
```

<= br>
And here is one more remark about the patch itself.
=
On Sat= , 21 Mar 2020 at 21:59, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

=C2=A0local function this_is_master()
@@ -2169,8 +2276,12 @@ local function storage_cfg(cfg, this_replica_uuid, i= s_reload)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error(err)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0end
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0log.info("Box has been configured") -=C2=A0 =C2=A0 =C2=A0 =C2=A0 local uri =3D luri.parse(this_replica.uri)
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 box.once("vshard:storage:1", storage= _schema_v1, uri.login, uri.password)
+=C2=A0 =C2=A0 end
+
+=C2=A0 =C2=A0 local uri =3D luri.parse(this_replica.uri)
+=C2=A0 =C2=A0 schema_upgrade(is_master, uri.login, uri.password)
+
+=C2=A0 =C2=A0 if not is_reload then

It= seems like this `if/else` statement isn't necessary. The `else` branch= is enough for both cases.
Even it's not hot reload it would = result in `local old =3D nil; box.space._bucket:on_replace(new_trigger, nil= )` which is essentially the same
=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0box.space._bucket:on_replace(bucket_gener= ation_increment)
=C2=A0 =C2=A0 =C2=A0else
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0local old =3D box.space._bucket:on_replac= e()[1]
@@ -2469,6 +2580,8 @@ M.rlist =3D {
=C2=A0 =C2=A0 =C2=A0add_tail =3D rlist_add_tail,
=C2=A0 =C2=A0 =C2=A0remove =3D rlist_remove,
=C2=A0}
+M.schema_latest_version =3D schema_latest_version
+M.schema_current_version =3D schema_current_version

=C2=A0return {
=C2=A0 =C2=A0 =C2=A0sync =3D sync,
--
2.21.1 (Apple Git-122.3)



--
=D0=A1 =D1=83= =D0=B2=D0=B0=D0=B6=D0=B5=D0=BD=D0=B8=D0=B5=D0=BC.=C2=A0
=D0=94=D1=8B=D0= =BD=D0=BD=D0=B8=D0=BA=D0=BE=D0=B2 =D0=AF=D1=80=D0=BE=D1=81=D0=BB=D0=B0=D0= =B2.
--0000000000002eee7f05a19b4f83--