[tarantool-patches] Re: [PATCH 2/2] Complete module reload

[Alex Khatskevich <avkhatskevich@tarantool.org>]

> Hello. Thanks for the fixes! See 10 comments below and
> the commit with some new fixes on the branch.
your fixes are squashed
>
> 1. Please, declare this outdated_mt out of this function in a module
> local variable like it is done with ordinal replica/replicaset mt. Not 
> just
> __index, but entire mt. See comment 3 why.
>
Ok, I like this approach.
> 2. You did not outdate replica metatables. How did your tests pass then?
Added test for outdate replica object && addrd outdated = true attribute to
old replicaset and replica objects.
>
> 3. You do not need to search old_replica_mt and old_replicaset_mt 
> here. You do not
> need them at all. Just declare special object_outdate_mt and set this 
> metatable to
> the old objects. Not just __index, but entire metatable. So you can 
> get rid of
> this old mts search, old mt parameters.
>
Ok, I like this approach.
>> +    if old_replicasets then
>> +        outdate_replicasets(old_replicasets, outdate_delay)
>
> 4. Please, name it schedule_connections_outdate. Now the name
> confuses. It could be thought to deprecate the objects now, not
> after timeout.
I have deleted an intermediate function, so, now it looks like this
         util.async_task(outdate_delay, outdate_replicasets, 
old_replicasets)
And I suppose, renaming is redundant.
>> +local function copy_metatables()
>
> 5. Please, remove. This function is harmful and slow. You do not
> need to copy metatables. Use my recommendations above and think.
Done
>
>>
>> @@ -514,20 +579,17 @@ local function buildall(sharding_cfg, 
>> old_replicasets)
>
> 6. Unused parameter.
Deleted
>
>> +    outdate_replicasets = outdate_replicasets,
>
> 7. Why do you need to export this function?
Typo. Fixed
>
>> +    local current_cfg = table.deepcopy(cfg)
>
> 8. Ok, I understand why do you copy it, but please,
> write a comment about it.
I decided to make a copy just for box.cfg and use `cfg` where it is
needed instead. It makes more sense for me and do not require extra
comments.
>> -M.discovery_f = discovery_f
>> -M.failover_f = failover_f
>> -
>> -if not rawget(_G, '__module_vshard_router') then
>> -    rawset(_G, '__module_vshard_router', M)
>> +if not rawget(_G, MODULE_INTERNALS) then
>> +    rawset(_G, MODULE_INTERNALS, M)
>>   else
>> +    router_cfg(M.current_cfg)
>>       M.module_version = M.module_version + 1
>>   end
>>
>> +M.discovery_f = discovery_f
>> +M.failover_f = failover_f
>
> 9. Why did you move these lines?
>
Because M table should not be changed in case of
`router_cfg(M.current_cfg)`
failure
>> +
>>   return {
>>       cfg = router_cfg;
>>       info = router_info;
>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
>> index 879c7c4..50347b0 100644
>> --- a/vshard/storage/init.lua
>> +++ b/vshard/storage/init.lua
>> @@ -21,6 +33,8 @@ if not M then
>>           -- See format in replicaset.lua.
>>           --
>>           replicasets = nil,
>> +        -- Time to deprecate old objects on reload.
>> +        reload_deprecate_timeout = nil,
>
> 10. Why here is 'reload_deprecate_timeout', but 
> 'connection_outdate_delay'
> in router?
Typo.
storage do not have such parameter at all. Deleted.