Re: [Tarantool-patches] [PATCH v23 3/3] test: add gh-6036-qsync-order test

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

On Fri, Oct 22, 2021 at 12:06:09AM +0200, Vladislav Shpilevoy wrote:
> > + | ...
> > +test_run:wait_cond(function() return box.error.injection.get("ERRINJ_WAL_WRITE_COUNT") > write_cnt end)
> 
> Sorry, keep the code inside of 80 symbols line width.

OK

> 
> I added this diff and all the tests passed. That means the only
> tested parts of the patchset are apply_synchro_row() and
> txn_limbo_is_replica_outdated().
> 

Without the rest of locking the overall picture becomes incomplete,
we introduce the first part of split brain detection. And in second
part we'are about to introduce filtering into begin()/commit() pair.

> Please, lets try not to submit more untested code. It is enough that
> we already have one ticket which simply adds assert(false) into a few
> places and the tests pass. It does not make the code better, it just
> adds more uncertainty how it works and whether it works at all.

This is the correct way to guard access to some particular data to me.
In this case we're guarding @promote_term_map and @promote_greatest_term.
What you propose is to guard them in "some path" but leave untouched for
all others. So the person which would read this code after some years
gonna be scratching a head trying to figure out why sometimes access to
these members is done locked and somtimes are not. I completely disagree
here. Either we lock all accesses either not, not partial lockings please.