Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladimir Davydov <vdavydov@tarantool.org>,
Subject: Re: [Tarantool-patches] [PATCH] net.box: drop conn:timeout()
Date: Tue, 27 Jul 2021 00:37:17 +0200	[thread overview]
Message-ID: <e4874f7e-562f-9862-b519-ed5f23eae845@tarantool.org> (raw)
In-Reply-To: <e6bf1f3347bce8ebc07a66279e2eb44fc8b11edc.1627297005.git.vdavydov@tarantool.org>

Hi! Thanks for the patch!

I hope I was supposed to review this. Could you please add the
reviewers into CC/To for next patches?

See 5 comments below.

On 26.07.2021 12:58, Vladimir Davydov via Tarantool-patches wrote:
> conn:timeout(timeout) writes (now + timeout) to conn._deadlines table
> keyed by fiber.self(). The deadline will then be used by all conn
> methods unless specified explicitly in the options argument.
> Usage of fiber.self() degrades net.box performance by about 15% in case
> timeout is not specified, because it is a C call and so interferes with
> JIT. See the ticket for the test details.
> Since the feature is unusable and was deprecated more than four years
> ago (in 1.7.4) in favor of specifying timeout in the options argument
> (see commit 58da206c33197), it should be fine to drop it.

1. Is it possible to add the commit title after the hash in quotes and
parentheses? Like this:

	(see commit 58da206c33197 ("net.box: add { timeout = } option to requests"))

It helps to quickly understand what was the commit about without
copy-pasting the hash for `git show`.

> Closes #6242
> ---

2. May I ask you please to push the patch onto a branch and attach the
issue and branch links after `---` in the first/cover email? For next
patches and for this one too. It simplifies the review; makes your patch
pass CI before the review (or not pass); and simplifies patch push in the

3. The "feature" is still documented:

You need to add a docbot request to the commit message for the
documentation team to remove all the mentionings of the old API.

For the syntax you can look at

For examples: grep by `TarantoolBot` in git log.

> diff --git a/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md
> new file mode 100644
> index 000000000000..69d12fb858e5
> --- /dev/null
> +++ b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md
> @@ -0,0 +1,9 @@
> +## feature/core
> +
> +* **[Breaking change]** timeout() method of net.box connection, which was
> +  marked deprecated more than four years ago (in 1.7.4) was dropped, because
> +  it negatively affected performance of hot net.box methods, like call() and
> +  select(), in case those are called without specifying a timeout.

4. Wouldn't it be hard, please, to reference the issue in the end
of the text using `(gh-####)` format? See other changelogs for examples.

> +
> +---
> +Breaking change: timeout() method of net.box connection was dropped.
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 3878abf21914..15e6c093cc15 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1289,28 +1280,11 @@ end
>  function remote_methods:wait_state(state, timeout)
>      check_remote_arg(self, 'wait_state')
> -    if timeout == nil then
> -        local deadline = self._deadlines[fiber_self()]
> -        timeout = deadline and max(0, deadline - fiber_clock())
> -    end
>      return self._transport.wait_state(state, timeout)
>  end
>  local compat_warning_said = false
> --- @deprecated since 1.7.4
> -function remote_methods:timeout(timeout)
> -    check_remote_arg(self, 'timeout')
> -    if not compat_warning_said then

5. compat_warning_said can be deleted now.

In the future you can find such remnants using luacheck, which was
integrated into our CI and can be run locally. For that you need
to install Tarantool into any folder, install luacheck rock, and
then you can do something like this:

	$ ./.rocks/bin/luacheck --codes --config .luacheckrc src/box/lua/net_box.lua
	Checking src/box/lua/net_box.lua                  1 warning

   		 src/box/lua/net_box.lua:1286:7: (W211) unused variable compat_warning_said

  reply	other threads:[~2021-07-26 22:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 10:58 Vladimir Davydov via Tarantool-patches
2021-07-26 22:37 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-07-27 10:06   ` Vladimir Davydov via Tarantool-patches
2021-07-27 21:27     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-29  8:05     ` Vladimir Davydov 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=e4874f7e-562f-9862-b519-ed5f23eae845@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=vdavydov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] net.box: drop conn:timeout()' \


* 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