From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Olga Arkhangelskaia <krishtal.olja@gmail.com> Cc: tarantool-patches@freelists.org Subject: Re: [tarantool-patches] [PATCH v4 2/2] box: adds replication sync after cfg. update Date: Tue, 28 Aug 2018 18:58:04 +0300 [thread overview] Message-ID: <20180828155804.j6uqt2gobzejpa6m@esperanza> (raw) In-Reply-To: <20180828114328.25702-2-krishtal.olja@gmail.com> 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. > 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. 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 = require('test_run') > +test_run = env.new() > +engine = test_run:get_cfg('engine') > + > +box.schema.user.grant('guest', 'replication') > + > +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'") > +test_run:cmd("start server replica") > + > +s = box.schema.space.create('test', {engine = engine}) > +index = s:create_index('primary') > + > +-- change replica configuration > +test_run:cmd("switch replica") > +box.cfg{replication_sync_lag = 0.1} > +replication = box.cfg.replication > +box.cfg{replication={}} > + > +test_run:cmd("switch default") > +-- insert values on the master while replica is unconfigured > +a = 3000 box.begin() while a > 0 do a = a-1 box.space.test:insert{a,a} end box.commit() Nit: for i = 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. 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. And a test case that checks that tarantool enters the orphan mode if it fails to sync. Please add. > + > +test_run:cmd("switch replica") > +box.cfg{replication = replication} > + > +box.space.test:count() == 3000 Nit: better do box.space.test:count() -- 3000 The reject file will be more informative in case of error then. > + > +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')
next prev parent reply other threads:[~2018-08-28 15:58 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-28 11:43 [tarantool-patches] [PATCH 1/2] box: make replication_sync_lag option dynamic Olga Arkhangelskaia 2018-08-28 11:43 ` [tarantool-patches] [PATCH v4 2/2] box: adds replication sync after cfg. update Olga Arkhangelskaia 2018-08-28 15:58 ` Vladimir Davydov [this message] 2018-08-28 16:19 ` Olga Krishtal 2018-08-28 16:36 ` Vladimir Davydov 2018-08-28 14:03 ` [tarantool-patches] [PATCH 1/2] box: make replication_sync_lag option dynamic Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180828155804.j6uqt2gobzejpa6m@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=krishtal.olja@gmail.com \ --cc=tarantool-patches@freelists.org \ --subject='Re: [tarantool-patches] [PATCH v4 2/2] box: adds replication sync after cfg. update' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox