Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH 1/1] iproto: get iproto obuf only right before usage
Date: Tue, 20 Mar 2018 16:07:35 +0300	[thread overview]
Message-ID: <20180320130735.xujvp3pc52d5djsj@esperanza> (raw)
In-Reply-To: <ce4fd164e4053830cba98052ff92ec1c28c2b35b.1521211045.git.v.shpilevoy@tarantool.org>

On Fri, Mar 16, 2018 at 05:41:42PM +0300, Vladislav Shpilevoy wrote:
> --- Branch: https://github.com/tarantool/tarantool/tree/fix-iproto-buffers
> --- There is no issue, since the fix is simple, and does not
> --- affect external behavior.

Please open a Github ticket. Just describe the problem there. A test
case is not necessary. This will be useful for release changelog.
(FYI I asked both Kostja and Kirill - they both agree).

> 
> --- Changes: do not rely on buffers number and rotation. Just
> --- send multiple fast read/write requests with a small delay.
> --- With no patch the test hangs or crashes since some responses
> --- are corrupted or discarded.

When I talked about extra info you don't want to be included into the
commit message, I meant the '---' generated by git-format-patch, as
everything following '---' is ignored by git-am.

Also, please don't forget to add the version to the subject line
(--subject-prefix='PATCH v2').

> 
> It is possible to discard non-sent responses using a special
> sequence of requests and yields. In details: if DML requests
> yield on commit too long, there are fast read requests, and a
> network is saturated, then some non-sent DML responses are
> discarded.
> 
> Signed-off-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

We don't sign patches, not yet. Please fix your git config.

> @@ -1175,8 +1175,15 @@ tx_process1(struct cmsg *m)
>  
>  	struct tuple *tuple;
>  	struct obuf_svp svp;
> -	if (box_process1(&msg->dml, &tuple) ||
> -	    iproto_prepare_select(out, &svp))

> +	ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
> +		if (rand() % 100 < 10)
> +			fiber_sleep(0.001);
> +	});
> +	struct obuf *out;
> +	if (box_process1(&msg->dml, &tuple) != 0)
> +		goto error;
> +	out = msg->connection->tx.p_obuf;
> +	if (iproto_prepare_select(out, &svp) != 0)
>  		goto error;
>  	if (tuple && tuple_to_obuf(tuple, out))
>  		goto error;
> @@ -1192,7 +1199,7 @@ static void
>  tx_process_select(struct cmsg *m)
>  {
>  	struct iproto_msg *msg = tx_accept_msg(m);
> -	struct obuf *out = msg->connection->tx.p_obuf;
> +	struct obuf *out;
>  	struct obuf_svp svp;
>  	struct port port;
>  	int count;
> @@ -1204,11 +1211,18 @@ tx_process_select(struct cmsg *m)
>  	if (tx_check_schema(msg->header.schema_version))
>  		goto error;
>  
> +	ERROR_INJECT(ERRINJ_IPROTO_TX_DELAY, {
> +		/* Select can yield, for example, on vinyl. */
> +		if (rand() % 100 < 10)
> +			fiber_sleep(0.001);
> +	});

There are quite a few constants in this error injection so
copying-and-pasting doesn't look good. I'd wrap it into an inline
function (tx_inject_delay or something, think of the name pls).

> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> index 9bcf53847..a24b357e6 100644
> --- a/test/box/errinj.test.lua
> +++ b/test/box/errinj.test.lua
> @@ -348,3 +348,19 @@ box.error.injection.set('ERRINJ_BUILD_SECONDARY', sk.id)
>  sk:alter({parts = {2, 'number'}})
>  box.error.injection.set('ERRINJ_BUILD_SECONDARY', -1)
>  s:drop()

Please add a brief comment explaining what and why we are testing here.

> +
> +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> +s = box.schema.space.create('test')
> +_ = s:create_index('pk')
> +
> +c = net_box.connect(box.cfg.listen)
> +
> +ch = fiber.channel(200)
> +errinj.set("ERRINJ_IPROTO_TX_DELAY", true)
> +for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:replace{1} end ch:put(true) end) end
> +for i = 1, 100 do fiber.create(function() for j = 1, 10 do c.space.test:select() end ch:put(true) end) end
> +for i = 1, 200 do ch:get() end
> +errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
> +
> +s:drop()
> +box.schema.user.revoke('guest', 'read,write,execute','universe')

  reply	other threads:[~2018-03-20 13:07 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20180316135103.usjz6veqe5mpste6@esperanza>
2018-03-16 14:41 ` Vladislav Shpilevoy
2018-03-20 13:07   ` Vladimir Davydov [this message]
2018-03-20 13:43     ` [PATCH v3 " Vladislav Shpilevoy
2018-03-20 16:49       ` Vladimir Davydov

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=20180320130735.xujvp3pc52d5djsj@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [PATCH 1/1] iproto: get iproto obuf only right before usage' \
    /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