<HTML><BODY>Please take a look at the new version.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 15 марта 2018, 17:10 +03:00 от Konstantin Osipov <kostja@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15211230370000000415_BODY">* Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>> [18/03/15 11:39]:<br><br>
The patch has to go to 1.6, since that's where the customer<br>
complained about it.<br><br>
Is it feasible to backport?</div></div></div></div></blockquote>for 1.9 branch: gh-3210-recover-missing-local-data-master-master<br>for 1.6 branch: gh-3210-recover-missing-local-data-master-master-16 *<br><br>* Right now, branch for 1.6 does not contain test, to do it first need to update test_run.lua<br>to support cluster configuration at 1.6<br>I think it could be useful in future as it makes adding/back-porting tests from 1.9 easier.<br>But we use test_run.lua as a reference to specific version. So two stage:<br>Update test_run and update all references.<br>@locker think it's waste of time and not necessary. Partially agree.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15211230370000000415_BODY"><br><br>
A few minor comment below.<br><br>
> @@ -110,6 +110,8 @@ struct relay {<br>
> struct vclock recv_vclock;<br>
> /** Replicatoin slave version. */<br>
> uint32_t version_id;<br>
> + /** lsn on subscribe, see #3210 */<br>
> + int64_t lsn_on_subscribe;<br><br>
I think it's *at* subscribe, not *on*. I would also identify<br>
which instance's LSN it is - it's LSN of this instance at<br>
the moment it has issued 'SUBSCRIBE' request. <br><br>
How about calling the variable 'local_lsn_before_subscribe' and<br>
describing the nature of the variable and the problem in the<br>
comment?</div></div></div></div></blockquote>Fixed<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15211230370000000415_BODY"><br><br>
> /** Relay endpoint */<br>
> struct cbus_endpoint endpoint;<br>
> @@ -541,6 +543,7 @@ relay_subscribe(int fd, uint64_t sync, struct replica *replica,<br>
> relay.version_id = replica_version_id;<br>
> relay.replica = replica;<br>
> replica_set_relay(replica, &relay);<br>
> + relay.lsn_on_subscribe = vclock_get(&replicaset.vclock, replica->id);<br>
> <br>
> int rc = cord_costart(&relay.cord, tt_sprintf("relay_%p", &relay),<br>
> relay_subscribe_f, &relay);<br>
> @@ -586,7 +589,8 @@ relay_send_row(struct xstream *stream, struct xrow_header *packet)<br>
> * (i.e. don't send replica's own rows back).<br>
> */<br>
> if (relay->replica == NULL ||<br>
> - packet->replica_id != relay->replica->id) {<br>
> + packet->replica_id != relay->replica->id ||<br>
> + packet->lsn <= relay->lsn_on_subscribe) {<br><br>
I think this change needs a comment. </div></div></div></div></blockquote>Done<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15211230370000000415_BODY"><br><br>
> relay_send(relay, packet);<br>
> }<br>
> }<br>
> diff --git a/src/box/wal.cc b/src/box/wal.cc<br>
> index 4576cfe09..5c87f08df 100644<br>
> --- a/src/box/wal.cc<br>
> +++ b/src/box/wal.cc<br>
> @@ -768,8 +768,14 @@ wal_write(struct journal *journal, struct journal_entry *entry)<br>
> /*<br>
> * Find last row from local instance id<br>
> * and promote vclock.<br>
> + * In master-master configuration, if some data<br>
> + * are missing on local master, others will send<br>
> + * them back. In this case replicaset vclock is<br>
> + * already set, so we should not update it.<br>
> + * See #3210 for details.<br><br>
I don't understand this comment, especially why the replicaset<br>
vclock is already set in this case. Could we extend it? </div></div></div></div></blockquote>Update to make it more clear<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15211230370000000415_BODY"><br><br>
Other than the comments and names of the variables, the patch<br>
looks good to me. Please let's discuss comments on telegram, or<br>
discuss them face to face with @locker.<br><br>
> */<br>
> - if ((*last)->replica_id == instance_id) {<br>
> + if ((*last)->replica_id == instance_id &&<br>
> + replicaset.vclock.lsn[instance_id] < (*last)->lsn) {<br><br>
> vclock_follow(&replicaset.vclock, instance_id,<br>
> (*last)->lsn);<br>
> break;<br>
> diff --git a/test/replication/master1.lua b/test/replication/master1.lua<br>
> new file mode 120000<br>
> index 000000000..16399ae47<br>
> --- /dev/null<br>
> +++ b/test/replication/master1.lua<br>
> @@ -0,0 +1 @@<br>
> +master_master.lua<br>
> \ No newline at end of file<br>
> diff --git a/test/replication/master2.lua b/test/replication/master2.lua<br>
> new file mode 120000<br>
> index 000000000..16399ae47<br>
> --- /dev/null<br>
> +++ b/test/replication/master2.lua<br>
> @@ -0,0 +1 @@<br>
> +master_master.lua<br>
> \ No newline at end of file<br>
> diff --git a/test/replication/master_master.lua b/test/replication/master_master.lua<br>
> new file mode 100644<br>
> index 000000000..ff165c367<br>
> --- /dev/null<br>
> +++ b/test/replication/master_master.lua<br>
> @@ -0,0 +1,30 @@<br>
> +#!/usr/bin/env tarantool<br>
> +<br>
> +-- get instance name from filename (master1.lua => master1)<br>
> +local INSTANCE_ID = string.match(arg[0], "%d")<br>
> +local USER = 'cluster'<br>
> +local PASSWORD = 'somepassword'<br>
> +local SOCKET_DIR = require('fio').cwd()<br>
> +local function instance_uri(instance_id)<br>
> + --return 'localhost:'..(3310 + instance_id)<br>
> + return SOCKET_DIR..'/master'..instance_id..'.sock';<br>
> +end<br>
> +<br>
> +-- start console first<br>
> +require('console').listen(os.getenv('ADMIN'))<br>
> +<br>
> +box.cfg({<br>
> + listen = instance_uri(INSTANCE_ID);<br>
> + replication = {<br>
> + USER..':'..PASSWORD..'@'..instance_uri(1);<br>
> + USER..':'..PASSWORD..'@'..instance_uri(2);<br>
> + };<br>
> +})<br>
> +<br>
> +box.once("bootstrap", function()<br>
> + local test_run = require('test_run').new()<br>
> + box.schema.user.create(USER, { password = PASSWORD })<br>
> + box.schema.user.grant(USER, 'replication')<br>
> + box.schema.space.create('test', {engine = test_run:get_cfg('engine')})<br>
> + box.space.test:create_index('primary')<br>
> +end)<br>
> diff --git a/test/replication/recover_missing.result b/test/replication/recover_missing.result<br>
> new file mode 100644<br>
> index 000000000..605232be4<br>
> --- /dev/null<br>
> +++ b/test/replication/recover_missing.result<br>
> @@ -0,0 +1,78 @@<br>
> +env = require('test_run')<br>
> +---<br>
> +...<br>
> +test_run = env.new()<br>
> +---<br>
> +...<br>
> +SERVERS = { 'master1', 'master2' }<br>
> +---<br>
> +...<br>
> +-- Start servers<br>
> +test_run:create_cluster(SERVERS)<br>
> +---<br>
> +...<br>
> +-- Wait for full mesh<br>
> +test_run:wait_fullmesh(SERVERS)<br>
> +---<br>
> +...<br>
> +test_run:cmd("switch master1")<br>
> +---<br>
> +- true<br>
> +...<br>
> +box.space._schema:insert({'1'})<br>
> +---<br>
> +- ['1']<br>
> +...<br>
> +box.space._schema:select('1')<br>
> +---<br>
> +- - ['1']<br>
> +...<br>
> +fiber = require('fiber')<br>
> +---<br>
> +...<br>
> +fiber.sleep(0.1)<br>
> +---<br>
> +...<br>
> +test_run:cmd("switch master2")<br>
> +---<br>
> +- true<br>
> +...<br>
> +box.space._schema:select('1')<br>
> +---<br>
> +- - ['1']<br>
> +...<br>
> +test_run:cmd("stop server master1")<br>
> +---<br>
> +- true<br>
> +...<br>
> +fio = require('fio')<br>
> +---<br>
> +...<br>
> +fio.unlink(fio.pathjoin(fio.abspath("."), string.format('master1/%020d.xlog', 8)))<br>
> +---<br>
> +- true<br>
> +...<br>
> +test_run:cmd("start server master1")<br>
> +---<br>
> +- true<br>
> +...<br>
> +test_run:cmd("switch master1")<br>
> +---<br>
> +- true<br>
> +...<br>
> +box.space._schema:select('1')<br>
> +---<br>
> +- - ['1']<br>
> +...<br>
> +test_run:cmd("switch default")<br>
> +---<br>
> +- true<br>
> +...<br>
> +test_run:cmd("stop server master1")<br>
> +---<br>
> +- true<br>
> +...<br>
> +test_run:cmd("stop server master2")<br>
> +---<br>
> +- true<br>
> +...<br>
> diff --git a/test/replication/recover_missing.test.lua b/test/replication/recover_missing.test.lua<br>
> new file mode 100644<br>
> index 000000000..2f36fc329<br>
> --- /dev/null<br>
> +++ b/test/replication/recover_missing.test.lua<br>
> @@ -0,0 +1,30 @@<br>
> +env = require('test_run')<br>
> +test_run = env.new()<br>
> +<br>
> +SERVERS = { 'master1', 'master2' }<br>
> +-- Start servers<br>
> +test_run:create_cluster(SERVERS)<br>
> +-- Wait for full mesh<br>
> +test_run:wait_fullmesh(SERVERS)<br>
> +<br>
> +test_run:cmd("switch master1")<br>
> +box.space._schema:insert({'1'})<br>
> +box.space._schema:select('1')<br>
> +<br>
> +fiber = require('fiber')<br>
> +fiber.sleep(0.1)<br>
> +<br>
> +test_run:cmd("switch master2")<br>
> +box.space._schema:select('1')<br>
> +test_run:cmd("stop server master1")<br>
> +fio = require('fio')<br>
> +fio.unlink(fio.pathjoin(fio.abspath("."), string.format('master1/%020d.xlog', 8)))<br>
> +test_run:cmd("start server master1")<br>
> +<br>
> +test_run:cmd("switch master1")<br>
> +box.space._schema:select('1')<br>
> +<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server master1")<br>
> +test_run:cmd("stop server master2")<br>
> +<br>
> -- <br>
> 2.14.3 (Apple Git-98)<br><br>
-- <br>
Konstantin Osipov, Moscow, Russia, <span class="js-phone-number">+7 903 626 22 32</span><br><a href="http://tarantool.org" target="_blank">http://tarantool.org</a> - <a href="http://www.twitter.com/kostja_osipov" target="_blank">www.twitter.com/kostja_osipov</a><br></div></div></div></div></blockquote>
<br>
<br>Best regards,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>