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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.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.