<div dir="ltr"><br>Thanks for the review! <div><br><div><div class="gmail_quote"><div dir="ltr">вт, 28 авг. 2018 г. в 18:58, Vladimir Davydov <<a href="mailto:vdavydov.dev@gmail.com">vdavydov.dev@gmail.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Aug 28, 2018 at 02:43:28PM +0300, Olga Arkhangelskaia wrote:<br>
> When replica reconnects to replica set not for the first time, we<br>
> suffer from absence of synchronization. Such behavior leads to giving<br>
> away outdated data.<br>
> <br>
> Closes #3427<br>
<br>
Please write a documentation request.<br></blockquote><div><br></div><div>Ok</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> diff --git a/src/box/box.cc b/src/box/box.cc<br>
> index be5077da8..aaae4219f 100644<br>
> --- a/src/box/box.cc<br>
> +++ b/src/box/box.cc<br>
> @@ -634,6 +634,11 @@ box_set_replication(void)<br>
> box_sync_replication(true);<br>
> /* Follow replica */<br>
> replicaset_follow();<br>
> + /* Sync replica up to quorum */<br>
> + if (!replicaset_sync()) {<br>
> + tnt_raise(ClientError, ER_CFG, "replication",<br>
> + "failed to connect to one or more replicas");<br>
> + }<br>
<br>
Throwing ER_CFG error from box.cfg() and still applying the new<br>
replication configuration looks weird. We should either revert the<br>
configuration back to what we had before box.cfg() was called or not<br>
throw exceptions.<br>
<br>
Reverting configuration seems to be unreasonable, because we could've<br>
applied some rows from the new replicas.<br>
<br>
We discussed the matter with Georgy and Kostja and agreed that instead<br>
an instance should enter the orphan mode, just like it does on initial<br>
configuration.<br>
<br></blockquote><div><br></div><div><br></div><div>Just curious, why? How can we applied changes if box.cfg throws an error? Or I miss smth?</div><div>Ok</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Sorry, we didn't come to an agreement earlier.<br>
<br>
Please rework and add a test case.<br>
<br>
> diff --git a/test/replication/sync.test.lua b/test/replication/sync.test.lua<br>
> new file mode 100644<br>
> index 000000000..4c2b55af8<br>
> --- /dev/null<br>
> +++ b/test/replication/sync.test.lua<br>
> @@ -0,0 +1,38 @@<br>
> +--<br>
> +-- gh-3427: no sync after configuration update<br>
> +--<br>
> +<br>
> +env = require('test_run')<br>
> +test_run = env.new()<br>
> +engine = test_run:get_cfg('engine')<br>
> +<br>
> +box.schema.user.grant('guest', 'replication')<br>
> +<br>
> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")<br>
> +test_run:cmd("start server replica")<br>
> +<br>
> +s = box.schema.space.create('test', {engine = engine})<br>
> +index = s:create_index('primary')<br>
> +<br>
> +-- change replica configuration<br>
> +test_run:cmd("switch replica")<br>
> +box.cfg{replication_sync_lag = 0.1}<br>
> +replication = box.cfg.replication<br>
> +box.cfg{replication={}}<br>
> +<br>
> +test_run:cmd("switch default")<br>
> +-- insert values on the master while replica is unconfigured<br>
> +a = 3000 box.begin() while a > 0 do a = a-1 box.space.test:insert{a,a} end box.commit()<br>
<br>
Nit: for i = 1, 100 do ... end </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Anyway, why 3000? When I change it to 1000 or even 100 the test still<br>
passes with this patch and fails without it.<br><br></blockquote><div><br></div><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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, I'd like to see a test case that checks that in case<br>
box.cfg.replication_sync_lag is big, not all records arrive<br>
by the time box.cfg{replication} returns.<br>
<br></blockquote><div><br></div><div>You mean see difference in tuples count in case when replicas are synced, however due to lag, but not due to data has arrived?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
And a test case that checks that tarantool enters the orphan mode<br>
if it fails to sync.<br>
<br>
Please add.<br></blockquote><div><br></div><div><br></div><div>Ok</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +test_run:cmd("switch replica")<br>
> +box.cfg{replication = replication}<br>
> +<br>
> +box.space.test:count() == 3000<br>
<br>
Nit: better do<br>
<br>
box.space.test:count() -- 3000<br>
<br>
The reject file will be more informative in case of error then.<br></blockquote><div><br></div><div>So I need 3 test case </div><div>Test that we are synced.</div><div>Test with sync and big lag.</div><div>Test with failed sync - orphan mode?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +test_run:cmd("switch default")<br>
> +<br>
> +-- cleanup<br>
> +test_run:cmd("stop server replica")<br>
> +test_run:cmd("cleanup server replica")<br>
> +box.space.test:drop()<br>
> +box.schema.user.revoke('guest', 'replication')<br>
</blockquote></div></div></div></div>