From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 43A4F6FC87; Fri, 1 Oct 2021 14:31:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 43A4F6FC87 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1633087869; bh=NagReb+Lx7h6TgNjcndEQ3oKOEe/XE1mGUYFkmOv/DM=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=JMNWS2j7lbujlDGCDrHjWq7q5lfMBl2nvOAcMXru23QtGO3EHsPVtk33T6fh02xsj Cx4AwqmjPhOFdE1l+hG6zXSSs5mFdGsdf6RP32VqeQjowa0eKga24st8OZhDNmuvDr PB7FZ/vbVBf+nF8CVnfApHUk/QxuAXytF/JzvvOE= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 303B06FC87 for ; Fri, 1 Oct 2021 14:31:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 303B06FC87 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1mWGkt-0005oJ-2K; Fri, 01 Oct 2021 14:31:07 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com References: <203dd4c5c23e717861a4952510882904323e10a0.1633028320.git.sergepetrenko@tarantool.org> <71d4862c-768a-3768-a36b-03487cfb5115@tarantool.org> Message-ID: <91aca699-9fbc-845f-0cc8-d4d2b09b8902@tarantool.org> Date: Fri, 1 Oct 2021 14:31:06 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <71d4862c-768a-3768-a36b-03487cfb5115@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD96A58C36AA2E99649E38D86FE9CCA94CF599BDB229C9CF279182A05F53808504046CD51B6AFA897141BCE4F9895DF960FAD4025AE0AD712F24CFAB43E6D4548D6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70ED3881ADD6CEF6AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063715F166F2542EEE4C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F2B540F0A204072DB61A119BD0462D34117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BB662CFBDBA8F60F475ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B505F01046CDA35AC58252B94C2ECE525A18 X-C1DE0DAB: 0D63561A33F958A52032F996D5D994EC3CEBEE4A7700CCEBCE4409AB2906F5E7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757AF5085B7B0228D6410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AE88D5ADEBE7C983982BEC9E4E3DC4C7DBCD2CB27880D6AFECC9EC7063EE4279A11CA740A498C14C1D7E09C32AA3244C00ABD718009960ECDABED00C72DF61B955E75C8D0ED9F6EE927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJNmX3owDPmEQ8PBtJ+U7qA== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446A2438847BEED9BCBE488496CB180AF651C484AA52A0C8A70424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/2] replication: fix replica disconnect upon reconfiguration X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 01.10.2021 01:15, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! > > See 6 comments below. > >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 219ffa38d..89cda5599 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc> @@ -1261,7 +1261,9 @@ box_sync_replication(bool connect_quorum) >> applier_delete(appliers[i]); /* doesn't affect diag */ >> }); >> >> - replicaset_connect(appliers, count, connect_quorum); >> + bool connect_quorum = strict; >> + bool keep_connect = !strict; >> + replicaset_connect(appliers, count, connect_quorum, keep_connect); > 1. How about passing both these parameters explicitly to box_sync_replication? > I don't understand the link between them so that they could be one. > > It seems the only case when you need to drop the old connections is when > you turn anon to normal. Why should they be fully reset otherwise? Yes, it's true. anon to normal is the only place where existing connections should be reset. For both bootstrap and local recovery (first ever box.cfg) keep_connect doesn't make sense at all, because there are no previous connections to keep. So the only two (out of 5) box_sync_replication calls, that need keep_connect are replication reconfiguration (keep_connect = true) and anon replica reconfiguration (keep_connect = false). Speaking of the relation between keep_connect and connect_quorum: We don't care about keep_connect in 3 calls (bootstrap and recovery), and when keep_connect is important, it's equal to !connect_quorum. I thought it might be nice to replace them with a single parameter. I tried to pass both parameters to box_sync_repication() at first. This looked rather ugly IMO: box_sync_replication(true, false), box_sync_replication(false, true); Two boolean parameters which are responsible for God knows what are worse than one parameter. I'm not 100% happy with my solution, but it at least hides the second parameter. And IMO box_sync_replication(strict) is rather easy to understand: when strict = true, you want to connect to quorum, and you want to reset the connections. And vice versa when strict = false. > >> diff --git a/src/box/replication.cc b/src/box/replication.cc >> index 1288bc9b1..e5fce6c8c 100644 >> --- a/src/box/replication.cc >> +++ b/src/box/replication.cc >> @@ -664,11 +681,11 @@ applier_on_connect_f(struct trigger *trigger, void *event) >> >> void >> replicaset_connect(struct applier **appliers, int count, >> - bool connect_quorum) >> + bool connect_quorum, bool keep_connect) >> { >> if (count == 0) { >> /* Cleanup the replica set. */ >> - replicaset_update(appliers, count); >> + replicaset_update(appliers, count, keep_connect); > 2. In case of count 0 it means all the appliers must be terminated, > mustn't they? So you could pass always false here. Up to you. Ok, let's change that. I'll change "count" to 0 as well. It has always bothered me here. ================================================= diff --git a/src/box/replication.cc b/src/box/replication.cc index e5fce6c8c..10b4ac915 100644 --- a/src/box/replication.cc +++ b/src/box/replication.cc @@ -685,7 +685,7 @@ replicaset_connect(struct applier **appliers, int count,  {         if (count == 0) {                 /* Cleanup the replica set. */ -               replicaset_update(appliers, count, keep_connect); +               replicaset_update(appliers, 0, false);                 return;         } ================================================= > >> return; >> } > 3. A few lines below I see that all the appliers are started via > applier_start and gather connections. Even if you have them already. > Wasn't the point of this patchset not to create connections when you > already have them? You could find matches by URI even before you > try to create a connection. It's true, but I decided it'd be simpler to find matches by replica uuid. It gives us a bonus: you won't drop an existing connection to a replica even when uri has changed. Say, "localhost:3303" and "127.0.0.1:3303". The point of the patchset is "don't restart existing connections if they are ok". Because if you restart them old relay doesn't exit in time and replica receives a "duplicate connection with same replica UUID". Here's how the patch works: 1. Get a list of appliers (one per each box.cfg.replication entry) 2. each applier is connected 3. new appliers receive master uuids 4. Find matches between new and old appliers, and remove duplicate    new appliers. (only when the old applier is functional) Here's how I thought I'd implement it at first: 1. you get a list of appliers 2. You check the new applier list against existing appliers 3. When you find matches, new appliers are removed. 4. Everything else works as before. The problem with the second approach is replicaset_update(). It always replaces all of the existing appliers with the new ones. How do we keep the old (matching) appliers then? Add them to new applier list? TBH I just decided that my approach would be simpler than this one. So there might be no problem at all. > > Otherwise when I do this: > > box.cfg{replication = {3313, 3314}} > box.cfg{replication = {3313, 3314, 3315}} > > you create 3 connections in the second box.cfg. While you > could create just 1. > >> diff --git a/src/box/replication.h b/src/box/replication.h >> index 4c82cd839..a8fed45e8 100644 >> --- a/src/box/replication.h >> +++ b/src/box/replication.h >> @@ -439,10 +439,12 @@ replicaset_add_anon(const struct tt_uuid *replica_uuid); >> * \param connect_quorum if this flag is set, fail unless at >> * least replication_connect_quorum >> * appliers have successfully connected. >> + * \param keep_connect if this flag is set do not force a reconnect if the >> + * old connection to the replica is fine. > 4. Why do you need to touch it even if the replica is not fine? > Shouldn't it reconnect automatically anyway? You could maybe force > it to reconnect right now if you don't want to wait until its > reconnect timeout expires. I don't understand this comment. Is it clear now after my other answers? P.S. I think I understand now. What if replication with the replica is permanently broken (by, say, ER_INVALID_MSGPACK)? I guess another `box.cfg{replication=...}` call should revive it. And yes, when replication_timeout is huge, and applier is waiting to retry for some reason, you might want to speed things up by another box.cfg{replication=...} call. > >> */ >> void >> replicaset_connect(struct applier **appliers, int count, >> - bool connect_quorum); >> + bool connect_quorum, bool keep_connect); >> >> diff --git a/test/replication-luatest/gh_4669_applier_reconnect_test.lua b/test/replication-luatest/gh_4669_applier_reconnect_test.lua >> new file mode 100644 >> index 000000000..62adff716 >> --- /dev/null >> +++ b/test/replication-luatest/gh_4669_applier_reconnect_test.lua >> @@ -0,0 +1,42 @@ >> +local t = require('luatest') >> +local fio = require('fio') >> +local Server = t.Server >> +local Cluster = require('test.luatest_helpers.cluster') > 5. Are we using first capital letters for variable names now? Maybe > stick to the guidelines and use lower case letters? I tried to stick to other luatest tests' style (replication-luatest on Vitaliya's branch). Sure, let's change that. > >> + >> +local g = t.group('gh-4669-applier-reconnect') >> + >> +local function check_follow_master(server) >> + return t.assert_equals( >> + server:eval('return box.info.replication[1].upstream.status'), 'follow') >> +end >> + >> +g.before_each(function() >> + g.cluster = Cluster:new({}) >> + g.master = g.cluster:build_server( >> + {}, {alias = 'master'}, 'base_instance.lua') >> + g.replica = g.cluster:build_server( >> + {args={'master'}}, {alias = 'replica'}, 'replica.lua') >> + >> + g.cluster:join_server(g.master) >> + g.cluster:join_server(g.replica) >> + g.cluster:start() >> + check_follow_master(g.replica) >> +end) >> + >> +g.after_each(function() >> + g.cluster:stop() >> +end) >> + >> +-- Test that appliers aren't recreated upon replication reconfiguration. >> +g.test_applier_connection_on_reconfig = function(g) >> + g.replica:eval( >> + 'box.cfg{'.. >> + 'replication = {'.. >> + 'os.getenv("TARANTOOL_LISTEN"),'.. >> + 'box.cfg.replication[1],'.. >> + '}'.. >> + '}' >> + ) > 6. Are we really supposed to write Lua code as Lua strings now? > You could use here [[ ... ]] btw, but the question still remains. > > Not a comment for your patch. But it looks unusable. What if the > test would be a bit more complicated? Look at qsync tests for instance. > Imagine writing all of them as Lua strings with quotes and all. > > Especially if you want to pass into the strings some parameters not > known in advance. I agree we need some way to access server's console directly. I don't know a good solution for this with luatest though. > > Could you maybe make it on top of master as a normal .test.lua test? I think not. Both Sergos and KirillY asked me to make this a luatest-test. Besides, this particular test looks ok in luatest. There's a single eval there. > >> + check_follow_master(g.replica) >> + t.assert_equals(g.master:grep_log("exiting the relay loop"), nil) >> +end >> Please, check out the diff. (I've renamed the test to `gh-4669-applier-reconnect_test.lua`. I tried `gh-4669-applier-reconnect.test.lua` first, but couldn't make luatest understand *.test.lua test names. I've asked Vitaliya to fix this) ======================================= diff --git a/src/box/replication.cc b/src/box/replication.cc index e5fce6c8c..10b4ac915 100644 --- a/src/box/replication.cc +++ b/src/box/replication.cc @@ -685,7 +685,7 @@ replicaset_connect(struct applier **appliers, int count,  {         if (count == 0) {                 /* Cleanup the replica set. */ -               replicaset_update(appliers, count, keep_connect); +               replicaset_update(appliers, 0, false);                 return;         } diff --git a/test/replication-luatest/gh_4669_applier_reconnect_test.lua b/test/replication-luatest/gh-4669-applier-reconnect_test.lua similarity index 74% rename from test/replication-luatest/gh_4669_applier_reconnect_test.lua rename to test/replication-luatest/gh-4669-applier-reconnect_test.lua index 62adff716..a4a138714 100644 --- a/test/replication-luatest/gh_4669_applier_reconnect_test.lua +++ b/test/replication-luatest/gh-4669-applier-reconnect_test.lua @@ -1,7 +1,7 @@  local t = require('luatest')  local fio = require('fio') -local Server = t.Server -local Cluster = require('test.luatest_helpers.cluster') +local server = t.Server +local cluster = require('test.luatest_helpers.cluster')  local g = t.group('gh-4669-applier-reconnect') @@ -11,7 +11,7 @@ local function check_follow_master(server)  end  g.before_each(function() -    g.cluster = Cluster:new({}) +    g.cluster = cluster:new({})      g.master = g.cluster:build_server(          {}, {alias = 'master'}, 'base_instance.lua')      g.replica = g.cluster:build_server( @@ -29,14 +29,14 @@ end)  -- Test that appliers aren't recreated upon replication reconfiguration.  g.test_applier_connection_on_reconfig = function(g) -    g.replica:eval( -        'box.cfg{'.. -            'replication = {'.. -                'os.getenv("TARANTOOL_LISTEN"),'.. -                'box.cfg.replication[1],'.. -            '}'.. -        '}' -    ) +    g.replica:eval([[ +        box.cfg{ +            replication = { +                os.getenv("TARANTOOL_LISTEN"), +                box.cfg.replication[1], +            } +        } +    ]])      check_follow_master(g.replica)      t.assert_equals(g.master:grep_log("exiting the relay loop"), nil)  end ======================================= -- Serge Petrenko