Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v23 3/3] test: add gh-6036-qsync-order test
Date: Sun, 24 Oct 2021 17:39:34 +0200	[thread overview]
Message-ID: <31fa78f8-c01b-27be-83a8-8eb9119a837a@tarantool.org> (raw)
In-Reply-To: <YXM1GtxA6Xb0kGWV@grain>

>> 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.

Sorry, but how is my comment above related to filtering? You introduced
some code which is dead - it is not tested at all. That was my only
point here.

It is not related to filtering - the ordering issue is separate as
you yourself noted when this was all a single patchset with the
split-brain detection. This is even proved by the test you added
here. I just need more tests for the other places which use the locking

>> 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.

Well, the only thing which I am worrying right now about is that if this
person in a few years will drop almost the entirety of your patchset
wondering whether it is needed really, all the tests will pass.

Also you seem to misunderstand my proposal. I didn't propose to make less
locking, I only proposed to increase it. I thought it will be easier to do
with full blocking of certain functions because it might be just easier to
get something locked in the tests. The bigger the critical section is - the
easier to hit it. Or maybe fine-grained locking works better if you say so.
Don't know.

It will only affect performance. Perf of qsync/raft now is 0, because
it is not used at all, due to being unsafe. Because of the reordering and
of the split-brain. This patch can't say it makes the reordering stuff much
better, because there are no tests except for a single case of reordering.

If from my diff you understood that I proposed to this drop code - sorry,
I didn't mean this. I meant that your patch should test these places, not
drop them. By commenting this code out I tried to prove that this code is
dead now, and that it shouldn't be so.

  reply	other threads:[~2021-10-24 15:39 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-14 21:56 [Tarantool-patches] [PATCH v23 0/3] qsync: implement packet filtering (part 1) Cyrill Gorcunov via Tarantool-patches
2021-10-14 21:56 ` [Tarantool-patches] [PATCH v23 1/3] latch: add latch_is_locked helper Cyrill Gorcunov via Tarantool-patches
2021-10-14 21:56 ` [Tarantool-patches] [PATCH v23 2/3] qsync: order access to the limbo terms Cyrill Gorcunov via Tarantool-patches
2021-10-21 22:06   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-14 21:56 ` [Tarantool-patches] [PATCH v23 3/3] test: add gh-6036-qsync-order test Cyrill Gorcunov via Tarantool-patches
2021-10-19 15:09   ` Serge Petrenko via Tarantool-patches
2021-10-19 22:26     ` Cyrill Gorcunov via Tarantool-patches
2021-10-20  6:35       ` Serge Petrenko via Tarantool-patches
2021-10-21 22:06         ` Vladislav Shpilevoy via Tarantool-patches
2021-10-22  6:36           ` Serge Petrenko via Tarantool-patches
2021-10-21 22:06   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-22 22:03     ` Cyrill Gorcunov via Tarantool-patches
2021-10-24 15:39       ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-10-24 16:01         ` Cyrill Gorcunov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=31fa78f8-c01b-27be-83a8-8eb9119a837a@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v23 3/3] test: add gh-6036-qsync-order test' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox