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 3D2E36EC40; Tue, 8 Jun 2021 21:52:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3D2E36EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1623178369; bh=S+9ONT9RTMOLk8QnBJaxsRlSNwDj/7lVQYgFu4+KyBs=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=wIbt/GNHA7zgNUFAtjR2g9ACLaEf1Dikoz9IaesrzWSPIxaOrnmi55IQd4CuOUV1b ubNsFybxwS6UuWi80/cmqoipAYsBKzZ6Kn1uQralL0Eus2DHU/3ZE639BhuCY1Bc4Z yYOdYm4DtaldByDYeeV9kAjY7QwWacVLTyI9uuP4= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 49FB56EC40 for ; Tue, 8 Jun 2021 21:52:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 49FB56EC40 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1lqgqF-0001Xu-Bo; Tue, 08 Jun 2021 21:52:47 +0300 To: Cyrill Gorcunov , tml References: <20210608180833.211678-1-gorcunov@gmail.com> Message-ID: <55c38f4a-7c07-4b15-f990-1d2965aedab6@tarantool.org> Date: Tue, 8 Jun 2021 20:52:46 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210608180833.211678-1-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9D5B0DA836B685C5407454A95E60932C8E3171F0D0805CD56182A05F538085040023B97B0FFF208D510B9A115C0F3FEF6F8604EE6697375D0CB9F2F372B37B3DB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79A02CDD1178524C2EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377BA6DB23C50317A38638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D842E4E5DC48E27255A32F4BB0673C07B4117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE7B96B19DC409332197A342136F30543AD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3AD74539164518AE5040F9FF01DFDA4A8C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006373BC478629CBEC79DEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24209795067102C07E8F7B195E1C97831A5D0385F63819417AAFB493F2837AFDE X-C1DE0DAB: 0D63561A33F958A594BF558FA289B1F02D4DBDAE4E4B1E2110E7882CF3861F12D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342F9EE325F82A28A8F38E5B47EE6743E97BA6AE1CFC2D8AA690F554C4485D3246A50ED106E19BCFBB1D7E09C32AA3244C71E63CC1572C33D1656B9CB2B377276AC3B3ADDA61883BB5FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojvdTwgM2ZyZnLN4tinfOD/A== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA31107336185F3AF10F64E34D9BD6ADA116CADBC91E654210EB0A07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5] applier: filter incoming synchro packets via transaction initiator 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Good job on the patch! See 7 comments below. On 08.06.2021 20:08, Cyrill Gorcunov wrote: > Currently we use synchro packets filtration based on their contents, > in particular by their xrow->replica_id value. Still there was a > question if we can optimize this moment and rather filter our all 1. our -> out. > packets coming from non-leader replica. > > diff --git a/test/replication/gh-6035-applier-filter.result b/test/replication/gh-6035-applier-filter.result > new file mode 100644 > index 000000000..cf032c332 > --- /dev/null > +++ b/test/replication/gh-6035-applier-filter.result > @@ -0,0 +1,147 @@ > +-- test-run result file version 2 > +-- > +-- gh-6035: verify synchronous rows filtration in applier, > +-- we need to be sure that filtering synchronous rows is > +-- done via transaction initiator not sender (iow via > +-- xrow->replica_id). > +-- > + > +local SOCKET_DIR = require('fio').cwd() > + | --- > + | ... > +local function unix_socket(name) \ > + return SOCKET_DIR .. "/" .. name .. '.sock' \ > +end 2. Both this function and SOCKET_DIR are not needed here. Because you declared them as 'local', and they go out of scope on the next line, and are deleted. In test-run's non-tap tests each line is a visibility scope. Like in the console. I deleted them from the test's code and the test passed. It works below because unix_socket() is already declared as global in your scripts calling box.cfg. Because it is global, you get luacheck warnings: Checking test/replication/gh6035node.lua 6 warnings test/replication/gh6035node.lua:4:10: (W111) setting non-standard global variable unix_socket test/replication/gh6035node.lua:12:18: (W113) accessing undefined variable unix_socket test/replication/gh6035node.lua:16:18: (W113) accessing undefined variable unix_socket test/replication/gh6035node.lua:18:13: (W113) accessing undefined variable unix_socket test/replication/gh6035node.lua:19:13: (W113) accessing undefined variable unix_socket test/replication/gh6035node.lua:27:13: (W113) accessing undefined variable unix_socket Maybe you could make the test not change box.cfg.replication? You could build the master <-> replica1 -> replica2 topology from the beginning. You start them as read-write, no elections. Replica1 would register on master. Replica2 would register on replcia1. Then you make master 'manual', 'replica1' voter, and 'replica2' voter. Then you promote the master and do the main part of the test. No box.cfg.replication was altered. > + | --- > + | ... > + > +env = require('test_run') > + | --- > + | ... > +test_run = env.new() 3. You don't need the env. You can do test_run = require('test_run').new() > + | --- > + | ... > +engine = test_run:get_cfg('engine') 4. You don't use the engine. Please, drop it, and make the test run only once, without 2 engines. > + | --- > + | ... > + > +test_run:cmd('create server gh6035master with script="replication/gh6035master.lua"') > + | --- > + | - true > + | ... > +test_run:cmd('start server gh6035master with args="gh6035master"') 5. Filename is already passed as arg[0] into the script. Look at autobootstrap.lua files to see how they extract the instance name from the file name. > + | --- > + | - true > + | ... > + > +test_run:cmd('create server gh6035replica1 with script="replication/gh6035replica1.lua"') > + | --- > + | - true > + | ... > +test_run:cmd('start server gh6035replica1 with args="gh6035replica1"') > + | --- > + | - true > + | ... > + > +test_run:cmd('create server gh6035replica2 with script="replication/gh6035replica2.lua"') > + | --- > + | - true > + | ... > +test_run:cmd('start server gh6035replica2 with args="gh6035replica2"') > + | --- > + | - true > + | ... > + > +-- > +-- The gh6035master node connected to gh6035replica1 and gh6035replica2, > +-- where each of gh6035replicaX connected to the gh6035master only. > +-- > +-- Lets reroute gh6035replica2 to gh6035replica1 so gh6035replica1 gonna > +-- be sending data to gh6035replica2 as a transit hop from gh6035master. > +test_run:cmd('switch gh6035replica2') 6. There is a shortcut 'test_run:switch()'. I also propose to drop gh6035 prefix from the instance names (but keep it in the script file names). It is hard to read otherwise. Another proposal: change gh6035 to gh-6035 in the file names. Otherwise they are too far from the test itself in the file explorer. > + | --- > + | - true > + | ... > +box.cfg{replication = {unix_socket("gh6035replica1")}} > + | --- > + | ... > + > +-- > +-- Make the gh6035master to be RAFT leader. > +test_run:cmd('switch gh6035master') > + | --- > + | - true > + | ... > +box.cfg{ \ > + replication = { \ > + unix_socket("gh6035master"), \ > + unix_socket("gh6035replica1"), \ > + }, \ > + replication_synchro_quorum = 2, \ > + election_mode = 'manual', \ > +} > + | --- > + | ... > +box.ctl.promote() > + | --- > + | ... > +_ = box.schema.space.create("sync", {is_sync = true}) > + | --- > + | ... > +_ = box.space.sync:create_index("pk") > + | --- > + | ... > +box.space.sync:insert{1} > + | --- > + | - [1] > + | ... > + > +-- The first hop is gh6035replica1. > +test_run:cmd('switch gh6035replica1') > + | --- > + | - true > + | ... > +box.space.sync:select{} > + | --- > + | - - [1] > + | ... > + > +-- And the second hop is gh6035replica2 where > +-- gh6035replica1 replicated the data to us. > +test_run:cmd('switch gh6035replica2') > + | --- > + | - true > + | ... > +box.space.sync:select{} > + | --- > + | - - [1] 7. You can't be sure it is replicated to replica2 yet. If replica1's disk is slow, it might need more time to write it to WAL. You need to use test_run:wait_lsn() to be sure that replica2 got the transaction. Wait_lsn() must make replica2 wait for all data from the master node, not from replica2. Because replica2 didn't write anything own.