From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20180828114328.25702-1-krishtal.olja@gmail.com> <20180828114328.25702-2-krishtal.olja@gmail.com> <20180828155804.j6uqt2gobzejpa6m@esperanza> In-Reply-To: <20180828155804.j6uqt2gobzejpa6m@esperanza> From: Olga Krishtal Date: Tue, 28 Aug 2018 19:19:55 +0300 Message-ID: Subject: Re: [tarantool-patches] [PATCH v4 2/2] box: adds replication sync after cfg. update Content-Type: multipart/alternative; boundary="000000000000e70a33057481372e" To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --000000000000e70a33057481372e Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks for the review! =D0=B2=D1=82, 28 =D0=B0=D0=B2=D0=B3. 2018 =D0=B3. =D0=B2 18:58, Vladimir Da= vydov : > On Tue, Aug 28, 2018 at 02:43:28PM +0300, Olga Arkhangelskaia wrote: > > When replica reconnects to replica set not for the first time, we > > suffer from absence of synchronization. Such behavior leads to giving > > away outdated data. > > > > Closes #3427 > > Please write a documentation request. > Ok > > > diff --git a/src/box/box.cc b/src/box/box.cc > > index be5077da8..aaae4219f 100644 > > --- a/src/box/box.cc > > +++ b/src/box/box.cc > > @@ -634,6 +634,11 @@ box_set_replication(void) > > box_sync_replication(true); > > /* Follow replica */ > > replicaset_follow(); > > + /* Sync replica up to quorum */ > > + if (!replicaset_sync()) { > > + tnt_raise(ClientError, ER_CFG, "replication", > > + "failed to connect to one or more replicas"); > > + } > > Throwing ER_CFG error from box.cfg() and still applying the new > replication configuration looks weird. We should either revert the > configuration back to what we had before box.cfg() was called or not > throw exceptions. > > Reverting configuration seems to be unreasonable, because we could've > applied some rows from the new replicas. > > We discussed the matter with Georgy and Kostja and agreed that instead > an instance should enter the orphan mode, just like it does on initial > configuration. > > Just curious, why? How can we applied changes if box.cfg throws an error? Or I miss smth? Ok > Sorry, we didn't come to an agreement earlier. > > Please rework and add a test case. > > > diff --git a/test/replication/sync.test.lua > b/test/replication/sync.test.lua > > new file mode 100644 > > index 000000000..4c2b55af8 > > --- /dev/null > > +++ b/test/replication/sync.test.lua > > @@ -0,0 +1,38 @@ > > +-- > > +-- gh-3427: no sync after configuration update > > +-- > > + > > +env =3D require('test_run') > > +test_run =3D env.new() > > +engine =3D test_run:get_cfg('engine') > > + > > +box.schema.user.grant('guest', 'replication') > > + > > +test_run:cmd("create server replica with rpl_master=3Ddefault, > script=3D'replication/replica.lua'") > > +test_run:cmd("start server replica") > > + > > +s =3D box.schema.space.create('test', {engine =3D engine}) > > +index =3D s:create_index('primary') > > + > > +-- change replica configuration > > +test_run:cmd("switch replica") > > +box.cfg{replication_sync_lag =3D 0.1} > > +replication =3D box.cfg.replication > > +box.cfg{replication=3D{}} > > + > > +test_run:cmd("switch default") > > +-- insert values on the master while replica is unconfigured > > +a =3D 3000 box.begin() while a > 0 do a =3D a-1 box.space.test:insert{= a,a} > end box.commit() > > Nit: for i =3D 1, 100 do ... end > Anyway, why 3000? When I change it to 1000 or even 100 the test still > passes with this patch and fails without it. > > I used 3000 because when there is no patch and I put replica into sleep for replication sync lag (0.1) arrives nearly 2500 tuples. > Also, I'd like to see a test case that checks that in case > box.cfg.replication_sync_lag is big, not all records arrive > by the time box.cfg{replication} returns. > > You mean see difference in tuples count in case when replicas are synced, however due to lag, but not due to data has arrived? > And a test case that checks that tarantool enters the orphan mode > if it fails to sync. > > Please add. > Ok > > > + > > +test_run:cmd("switch replica") > > +box.cfg{replication =3D replication} > > + > > +box.space.test:count() =3D=3D 3000 > > Nit: better do > > box.space.test:count() -- 3000 > > The reject file will be more informative in case of error then. > So I need 3 test case Test that we are synced. Test with sync and big lag. Test with failed sync - orphan mode? > > > + > > +test_run:cmd("switch default") > > + > > +-- cleanup > > +test_run:cmd("stop server replica") > > +test_run:cmd("cleanup server replica") > > +box.space.test:drop() > > +box.schema.user.revoke('guest', 'replication') > --000000000000e70a33057481372e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Thanks for the review!=C2=A0

=D0=B2=D1=82, 28 =D0=B0=D0=B2=D0=B3. 2018= =D0=B3. =D0=B2 18:58, Vladimir Davydov <vdavydov.dev@gmail.com>:
On Tue, Aug 28, 2018 at 02:43:28PM +0300, Olga A= rkhangelskaia wrote:
> When replica reconnects to replica set not for the first time, we
> suffer from absence of synchronization. Such behavior leads to giving<= br> > away outdated data.
>
> Closes #3427

Please write a documentation request.

O= k
=C2=A0

> diff --git a/src/box/box.cc b/src/box/box.cc
> index be5077da8..aaae4219f 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -634,6 +634,11 @@ box_set_replication(void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0box_sync_replication(true);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Follow replica */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0replicaset_follow();
> +=C2=A0 =C2=A0 =C2=A0/* Sync replica up to quorum */
> +=C2=A0 =C2=A0 =C2=A0if (!replicaset_sync()) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0tnt_raise(ClientError= , ER_CFG, "replication",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0"failed to connect to one or more replicas");
> +=C2=A0 =C2=A0 =C2=A0}

Throwing ER_CFG error from box.cfg() and still applying the new
replication configuration looks weird. We should either revert the
configuration back to what we had before box.cfg() was called or not
throw exceptions.

Reverting configuration seems to be unreasonable, because we could've applied some rows from the new replicas.

We discussed the matter with Georgy and Kostja and agreed that instead
an instance should enter the orphan mode, just like it does on initial
configuration.



Just curious, why?=C2= =A0 How can we applied changes if box.cfg throws an error? Or I miss smth?<= /div>
Ok

=C2=A0
Sorry, we didn't come to an agreement earlier.

Please rework and add a test case.

> diff --git a/test/replication/sync.test.lua b/test/replication/sync.te= st.lua
> new file mode 100644
> index 000000000..4c2b55af8
> --- /dev/null
> +++ b/test/replication/sync.test.lua
> @@ -0,0 +1,38 @@
> +--
> +-- gh-3427: no sync after configuration update
> +--
> +
> +env =3D require('test_run')
> +test_run =3D env.new()
> +engine =3D test_run:get_cfg('engine')
> +
> +box.schema.user.grant('guest', 'replication')
> +
> +test_run:cmd("create server replica with rpl_master=3Ddefault, s= cript=3D'replication/replica.lua'")
> +test_run:cmd("start server replica")
> +
> +s =3D box.schema.space.create('test', {engine =3D engine}) > +index =3D s:create_index('primary')
> +
> +-- change replica configuration
> +test_run:cmd("switch replica")
> +box.cfg{replication_sync_lag =3D 0.1}
> +replication =3D box.cfg.replication
> +box.cfg{replication=3D{}}
> +
> +test_run:cmd("switch default")
> +-- insert values on the master while replica is unconfigured
> +a =3D 3000 box.begin() while a > 0 do a =3D a-1 box.space.test:ins= ert{a,a} end box.commit()

Nit: for i =3D 1, 100 do ... end=C2=A0

Anyway, why 3000? When I change it to 1000 or even 100 the test still
passes with this patch and fails without it.


<= /div>
I used 3000 because when there is no patch and I put replica into= sleep for replication sync lag (0.1) arrives nearly 2500 tuples.

=C2=A0
Also, I'd like to see a test case that checks that in case
box.cfg.replication_sync_lag is big, not all records arrive
by the time box.cfg{replication} returns.


You mean see difference in tuples coun= t in case when replicas are synced, however due to lag, but not due to data= has arrived?
=C2=A0
And a test case that checks that tarantool enters the orphan mode
if it fails to sync.

Please add.


Ok
=C2=A0

> +
> +test_run:cmd("switch replica")
> +box.cfg{replication =3D replication}
> +
> +box.space.test:count() =3D=3D 3000

Nit: better do

box.space.test:count() -- 3000

The reject file will be more informative in case of error then.

So I need 3 test case=C2=A0
Test that we= are synced.
Test with sync and big lag.
Test with fail= ed sync - orphan mode?

=C2=A0

> +
> +test_run:cmd("switch default")
> +
> +-- cleanup
> +test_run:cmd("stop server replica")
> +test_run:cmd("cleanup server replica")
> +box.space.test:drop()
> +box.schema.user.revoke('guest', 'replication')
--000000000000e70a33057481372e--