Re: [Tarantool-patches] [PATCH vshard 1/1] rebalancer: give more info at bucket_recv() fail

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for your answers! LGTM.


On 26.05.2021 21:44, Vladislav Shpilevoy wrote:
> 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.
Thanks for explanation.


>>> +                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.
>
Yes, I missed it. But if it's lua table (not box.tuple) it's OK.


>> 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.
Ok, agree.