Re: [Tarantool-patches] [PATCH vshard 1/2] router: wrap is_async futures completely

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the review!

On 29.09.2021 08:18, Oleg Babin wrote:
> Thanks for your patch! I left three comments below.
> 
> On 29.09.2021 02:08, Vladislav Shpilevoy wrote:
>> Router used to override :result() method of netbox futures. It is
>> needed because user functions are called via vshard.storage.call()
>> which returns some metadata - it must be truncated before
>> returning the user's data.
>>
>> It worked fine while netbox futures were implemented as tables.
>> But in the newest Tarantool most of netbox state machine code is
>> moved into C. The futures now are cdata.
> AFAIK userdata (not cdata) if it makes sense.

Changed to 'C structs'.

>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>> index 3d468f3..42de740 100644
>> --- a/vshard/router/init.lua
>> +++ b/vshard/router/init.lua
>> @@ -470,6 +470,114 @@ end
>>   -- API
>>   --------------------------------------------------------------------------------
>>   
> 
> I thing following changes could be moved into sepearate file.

I thought of that, but decided it won't improve much. These futures are
a part of router_call, they can't be used for anything else. Because
are specific to how the storage answers to router_call.

They won't work for replicaset:call() nor for router_map_callrw()

So by moving them out I won't make it easier to reuse them or make
them more independent. I will just reduce init.lua file size, which
is not a primary goal.

I was rather hoping to move the entire router_call() into its own file
eventually with all its satellite structures and functions used only
by it.

Does it make sense?

>> +local function vshard_future_tostring(self)
>> +    return 'vshard.net.box.request'
>> +end
>> +
>> +local function vshard_future_serialize(self)
>> +    -- Drop the metatable. It is also copied and if returned as is leads to
>> +    -- recursive serialization.
>> +    local s = setmetatable(table.deepcopy(self), {})
>> +    s._base = nil
>> +    return s
>> +end
>> +
>> +local function vshard_future_is_ready(self)
>> +    return self._base:is_ready()
>> +end
>> +
>> +local function vshard_future_wrap_result(res)
>> +    local storage_ok, res, err = unpack(res)
> 
> Unpack could be replaced with ... = res[1], res[2], res[3].
> 
> Should be a bit faster since unpack can't be jitted.

Nice, didn't know that.

====================
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 42de740..4748398 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -487,7 +487,7 @@ local function vshard_future_is_ready(self)
 end
 
 local function vshard_future_wrap_result(res)
-    local storage_ok, res, err = unpack(res)
+    local storage_ok, res, err = res[1], res[2], res[3]
     if storage_ok then
         if res == nil and err ~= nil then
             return nil, lerror.make(err)