Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Oleg Babin <olegrok@tarantool.org>,
	tarantool-patches@dev.tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH vshard 1/1] rebalancer: give more info at bucket_recv() fail
Date: Wed, 26 May 2021 20:44:26 +0200	[thread overview]
Message-ID: <25369324-54cd-1611-c39a-0c4a1e8686a6@tarantool.org> (raw)
In-Reply-To: <7284b714-dfa7-026e-5979-2d5eb3e55386@tarantool.org>

Hi! Thanks for the review!

>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
>> index 63e0398..7045d91 100644
>> --- a/vshard/storage/init.lua
>> +++ b/vshard/storage/init.lua
>> @@ -1254,7 +1254,13 @@ local function bucket_recv_xc(bucket_id, from, data, opts)
>>           end
>>           box.begin()
>>           for _, tuple in ipairs(space_data) do
>> -            space:insert(tuple)
>> +            local ok, err = pcall(space.insert, space, tuple)
>> +            if not ok then
>> +                box.rollback()
> 
> Am I right that before a patch nobody rolled back transaction is case of error?
> 
> How did it work?

bucket_recv_xc() is called only from bucket_recv() via pcall.
Bucket_recv() does the rollback. I have the _xc() version so
as not to wrap into pcalls everything, and as a protection
against potential OOM. For instance, when I create a table
in there like `{bucket_id, recvg, from}` - it might fail too,
AFAIU.

>> +                return nil, lerror.vshard(lerror.code.BUCKET_RECV_DATA_ERROR,
>> +                                          bucket_id, space.name,
>> +                                          box.tuple.new(tuple), err)
>> +            end
> 
> Do you really need `box.tuple.new` here. Why just `tuple` is not enough?

Because `tuple` is a Lua table. When formatted into %s in the error
message, it turns into 'table 0x......' instead of showing the
content, while tuple objects have a nice serializer.

> AFAIU box.tuple.new doesn't just increment tuple ref-counter and construct new tuple.

It does exactly this.

> Rebalancing is quite CPU-intensive operation so I'm not sure that such behaviour doesn't
> 
> make error case worse.

I thought about it, but decided that it is not worth optimizing the
error case. It is better to provide a good error message. I also
thought about using json.encode() to avoid 'table 0x.....' problem,
but decided I don't want to introduce a dependency on the entire json
module just for this.

  reply	other threads:[~2021-05-26 18:44 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25 20:42 Vladislav Shpilevoy via Tarantool-patches
2021-05-26  9:03 ` Oleg Babin via Tarantool-patches
2021-05-26 18:44   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-05-27  8:24     ` Oleg Babin via Tarantool-patches
2021-05-27 19:20 ` Vladislav Shpilevoy 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=25369324-54cd-1611-c39a-0c4a1e8686a6@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH vshard 1/1] rebalancer: give more info at bucket_recv() fail' \
    /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