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

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

Hi! Thanks for your changes. LGTM.

On 01.10.2021 01:39, Vladislav Shpilevoy wrote:
> 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?

I thought about simplification, not about reusing. As for me it's simpler to

keep some logic into separate file. But I won't insist.