Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>,
	Konstantin Osipov <kostja.osipov@gmail.com>,
	gorcunov@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v3] wal: introduce limits on simultaneous writes
Date: Tue, 16 Mar 2021 00:42:22 +0100	[thread overview]
Message-ID: <54666e16-f577-c9f5-e917-f834b7e4b8d5@tarantool.org> (raw)
In-Reply-To: <d1179904-55ae-57ac-9454-554589d9c26d@tarantool.org>

Hi! Thanks for the patch!

I must admit, it looks kind of ugly :D. The class we have now only remotely
looks like a semaphore.

Number of reasons, some of which you already listed in the header file:

- It is advisory. Can be bypassed easily if you forget to check wouldblock.
  But not a big issue really. An optional thing like 'try_take' is needed
  for box.commit({is_async = true}) anyway, not to block the fiber;

- You can take more amount of the resource than there is. Bearable as well,
  but still;

- sem_release() does not wakeup anybody. Quite counter-intuitive;

- The wouldblock check not only checks the resource being available, but also
  if there are any waiters. It wouldn't matter for a real semaphore, because
  it has nothing to do with ordering the waiters in FIFO. It is a detail of
  the journal which slipped into the general class.
  But maybe that is the only way to make it fair? Otherwise some fibers
  could be blocked forever due to starvation.

The last thing I am not sure is even an issue. Might be a feature.

The others probably can be fixed if we would rework journal_queue API. For
instance, not have journal_queue_wait() separated from journal_queue_on_append().
Then sem_take() could become blocking and obligatory.

You simply inline everything into journal_write() and journal_write_try_async(),
and you will see that you can always call take() and block inside of it.

But I don't know if it is worth doing TBH. It is used in a single place so far.
This is hard to define fiber_sem API which would be suitable for future usages.
I would vote for not doing it now and see if we would need the semaphore in the
future.

Although the idea about removing journal_queue_wait() might be worth trying.
It is used either right before journal_queue_on_append(), or in
journal_queue_flush() which is also right before journal_queue_on_append().
Up to you. Anyway we need to return to this code for box.commit({is_async})
feature, which means the hard polishing might be not so useful.

  parent reply	other threads:[~2021-03-15 23:42 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-24 19:35 Serge Petrenko via Tarantool-patches
2021-02-24 19:40 ` Serge Petrenko via Tarantool-patches
2021-02-25 13:05 ` Konstantin Osipov via Tarantool-patches
2021-02-26  0:57   ` Vladislav Shpilevoy via Tarantool-patches
2021-02-26  7:18     ` Konstantin Osipov via Tarantool-patches
2021-02-26 20:23       ` Vladislav Shpilevoy via Tarantool-patches
2021-02-26 21:20         ` Konstantin Osipov via Tarantool-patches
2021-02-26 22:44           ` Vladislav Shpilevoy via Tarantool-patches
2021-02-27 13:27             ` Konstantin Osipov via Tarantool-patches
2021-03-01 19:15   ` Serge Petrenko via Tarantool-patches
2021-03-01 21:46     ` Konstantin Osipov via Tarantool-patches
2021-02-26  0:56 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 19:08   ` Serge Petrenko via Tarantool-patches
2021-03-01 22:05     ` Vladislav Shpilevoy via Tarantool-patches
2021-03-02 17:51       ` Serge Petrenko via Tarantool-patches
2021-03-03 20:59         ` Vladislav Shpilevoy via Tarantool-patches
2021-03-09 15:10           ` Serge Petrenko via Tarantool-patches
2021-03-09 19:49 ` Vladislav Shpilevoy via Tarantool-patches
2021-03-10  8:18   ` Konstantin Osipov via Tarantool-patches
2021-03-12 17:10     ` Serge Petrenko via Tarantool-patches
2021-03-13 19:14       ` Konstantin Osipov via Tarantool-patches
2021-03-15 23:42       ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-03-16  6:45         ` Konstantin Osipov via Tarantool-patches
2021-03-16 20:27           ` Vladislav Shpilevoy via Tarantool-patches
2021-03-16 10:19         ` Serge Petrenko via Tarantool-patches
2021-03-16 20:48           ` Vladislav Shpilevoy via Tarantool-patches
2021-03-17 12:14             ` Serge Petrenko via Tarantool-patches
2021-03-17 21:02           ` Vladislav Shpilevoy via Tarantool-patches
2021-03-19 11:32             ` Serge Petrenko via Tarantool-patches
2021-03-19 15:36 ` Kirill Yukhin 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=54666e16-f577-c9f5-e917-f834b7e4b8d5@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=kostja.osipov@gmail.com \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v3] wal: introduce limits on simultaneous writes' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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