Re: [Tarantool-patches] [PATCH v9 0/1] applier: filter incoming synchro packets via transaction initiator

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Jun 18, 2021 at 11:49:39PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 4 comments below.
> 
> > diff --git a/test/replication/gh-6035-applier-filter.result b/test/replication/gh-6035-applier-filter.result
> > new file mode 100644
> > index 000000000..7345a19f7
> > --- /dev/null
> > +++ b/test/replication/gh-6035-applier-filter.result
> 
> 1. Maybe better rename it to gh-6035-election-filter. Because otherwise
> this test is not included when I run `python test-run.py election`. And I
> do that quite often when make a patch which affects election only.

Sure, I don't mind.

> > +box.cfg{replication = {require('fio').cwd() .. "/replica1.sock"}}
> 
> 2. You don't need require('fio').cwd(). "unix/:./replica1.sock" works
> just fine. The same for the other socket paths.

Actually I too this from other examples since I suspect the use of
absolute path might be critical for test run engine. It is not a problem
to rename but other our tests do use this trick so meaybe we should stick
with same approach? Again, I don't mind to use relative path if this
won't cause problems in future.

> > +-- Make the master to be RAFT leader, this drops connection
> > +-- to the replica2.
> 
> 3. There was no connection to replica2 from master.
>

I'll update the comment, thanks! (actually replica2
connected to the master initialy and this is full
duplex connection which we close on reconfig, that's
what I meant saying "dropping" connection, but this
seems to be confusing).

> > +    "gh-6035-applier-filter.test.lua": {
> > +        "memtx": {"engine": "memtx"}
> > +    },
> 
> 4. You don't need to specify the engine. You don't use the
> engine variable in the test. Just leave it empty {}.

OK