Re: [Tarantool-patches] [PATCH v17 5/5] test: add gh-6036-term-order test

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

On Sun, Sep 26, 2021 at 04:30:38PM +0200, Vladislav Shpilevoy wrote:
> > +if INSTANCE_ID == "master" then
> > +    box.cfg({
> > +        listen                      = unix_socket(INSTANCE_ID),
> > +        replication                 = {
> > +            unix_socket(INSTANCE_ID),
> > +            unix_socket("replica1"),
> > +            unix_socket("replica2"),
> > +        },
> > +        replication_connect_quorum  = 1,
> > +        replication_synchro_quorum  = 1,
> > +        replication_synchro_timeout = 10000,
> > +        replication_sync_timeout    = 5,
> 
> 1. Why do you need sync_timeout 5?

To make sure it has some sane short value, our default
300 seconds is too big I think.

> > +
> > +--box.ctl.wait_rw()
> 
> 2. Please, remove commented out code.

ok

> > +box.once("bootstrap", function()
> > +    box.schema.user.grant('guest', 'super')
> > +end)
> > diff --git a/test/replication/gh-6036-term-order.result b/test/replication/gh-6036-term-order.result
> > new file mode 100644
> > index 000000000..6b19fc2c8
> > --- /dev/null
> > +++ b/test/replication/gh-6036-term-order.result
> 
> 3. Please, use prefix gh-####-qsync to be consistent with other qsync tests. Having
> 'qsync' in the test name helps to run all qsync tests in a single command
> 
> 	python test-run.py qsync

sure, will do

> > +test_run:switch("replica2")
> > + | ---
> > + | - true
> > + | ...
> > +box.ctl.demote()
> 
> 4. I dropped all 3 demotes and the test passed. Why do you need them?

To make sure none of the fresh booted up nodes are owning the limbo even
if something is changed in future inside test-run engine (test engine is
not a stable API while our demote() operation is part of API and I can
be sure that I may don't care how exactly nodes has been started, they
all won't be owning the limbo after this command).

> > +term_max_wait4 = term_max_master
> > + | ---
> > + | ...
> > +if term_max_wait4 < term_max_replica1 then term_max_wait4 = term_max_replica1 end
> 
> 5. How is it possible? The master did more promotes, it should have a
> bigger term for sure.

IIRC I've hunting a race buggy testcase and left this snippet untouched. So this snippet
simply sneaked in, it is harmless, I'll cleanup, thanks!

> 
> > +test_run:cmd('delete server replica2')
> 
> 6. Can you add some data to the test? Which before the patches was applied, and now
> is rejected/nopified. Otherwise you added a lock, tested the lock, but if I move
> the data filtering before the lock, your test still will pass.

Will try, thanks!