[Tarantool-patches] [PATCH vshard 1/1] rebalancer: give more info at bucket_recv() fail
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 26 21:44:26 MSK 2021
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.
More information about the Tarantool-patches
mailing list