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

[Alex Khatskevich <avkhatskevich@tarantool.org>]

>>
>> Part of <shut git>112
>
> 1. Stray '<shut git>'.
Ok
>
>> ---
>>   test/router/reload.result    | 159 
>> +++++++++++++++++++++++++++++++++++++++++++
>>   test/router/reload.test.lua  |  48 +++++++++++++
>>   test/router/router.result    |   3 +-
>>   test/storage/reload.result   |  68 ++++++++++++++++++
>>   test/storage/reload.test.lua |  23 +++++++
>>   vshard/cfg.lua               |   6 ++
>>   vshard/error.lua             |   5 ++
>>   vshard/replicaset.lua        | 100 ++++++++++++++++++++-------
>>   vshard/router/init.lua       |  44 ++++++++----
>>   vshard/storage/init.lua      |  45 ++++++++----
>>   vshard/util.lua              |  20 ++++++
>>   11 files changed, 466 insertions(+), 55 deletions(-)
>>
>> diff --git a/test/router/reload.result b/test/router/reload.result
>> index 47f3c2e..3fbbe6e 100644
>> --- a/test/router/reload.result
>> +++ b/test/router/reload.result
>> @@ -174,6 +174,165 @@ vshard.router.module_version()
>> ...
>> +-- Make sure outdate async task has had cpu time.
>> +fiber.sleep(0.005)
>
> 2. As I asked earlier, please, avoid constant timeouts.
> When you want to wait for something, use 'while'.
Fixed
>> +---
>> +...
>> +vshard.router = require('vshard.router')
>
> 3. You have already did it few lines above.
fixed
>> +-- Error during reconfigure process.
>
> 4. You added this test in the previous commit.
Thanks
>
>> +vshard.storage.internal.errinj.ERRINJ_CFG = true
>
> 5. Same as 4. We have already added the test in the previous
> commit, it is not?
thanks
>>
>> diff --git a/vshard/cfg.lua b/vshard/cfg.lua
>> index d5429af..87d0fc8 100644
>> --- a/vshard/cfg.lua
>> +++ b/vshard/cfg.lua
>> @@ -217,6 +217,10 @@ local cfg_template = {
>>           type = 'non-negative number', name = 'Sync timeout', 
>> is_optional = true,
>>           default = consts.DEFAULT_SYNC_TIMEOUT
>>       }},
>> +    {'connection_outdate_delay', {
>> +        type = 'non-negative number', name = 'Object outdate timeout',
>> +        is_optional = true, default = nil
>
> 6. default = nil makes no sense for Lua tables.
deleted
>
>> +    }},
>>   }
>>     --
>> @@ -264,6 +268,8 @@ local function remove_non_box_options(cfg)
>>       cfg.collect_bucket_garbage_interval = nil
>>       cfg.collect_lua_garbage = nil
>>       cfg.sync_timeout = nil
>> +    cfg.sync_timeout = nil
>
> 7. Why do you need the second nullify of sync_timeout?
rebase consecuence...
>
>> +    cfg.connection_outdate_delay = nil
>>   end
>>     return {
>> diff --git a/vshard/replicaset.lua b/vshard/replicaset.lua
>> index 99f59aa..ec6e95b 100644
>> --- a/vshard/replicaset.lua
>> +++ b/vshard/replicaset.lua> @@ -412,6 +424,49 @@ for name, func in 
>> pairs(replica_mt.__index) do
>>   end
>>   replica_mt.__index = index
>>   +--
>> +-- Meta-methods of outdated objects
>
> 8. Put the dot at the end of the sentence.
fixed
>
>> +-- They define only arrtibutes from corresponding metatables to
>
> 9. Typo: arrtibutes.
fixed
>
>> +-- make user able to access fields of old objects.
>> +--
>> +local function outdated_warning(...)
>> +    return nil, lerror.vshard(lerror.code.OBJECT_IS_OUTDATED)
>> +end
>> +
>> +local outdated_replicaset_mt = {
>> +    __index = {
>> +        outdated = true
>> +    }
>> +}
>
> 10. outdated_replicaset/replica_mt are identical. Please,
> make one mt object names 'outdated_mt'.
>
>> +for fname, func in pairs(replicaset_mt.__index) do
>> +    outdated_replicaset_mt.__index[fname] = outdated_warning
>> +end
>
> 11. As I remember, my proposal was to do not
> duplicate each method here, but just on any __index return
> callable object that on call invokes outdated_warning.
>
> You can ask, what to do with 'outdated' attribute then, but
> you can set it directly in the object in outdate_replicasets()
> function.
>
> What is more, please, use 'is_outdated' since this value is a
> flag. And add it to the description on top of the file, where
> all attributes are documented.
>
> Here you can put 'replica.is_outdated = true'.
>
> Here you can put 'replicaset.is_outdated = true'.
10, 11 - to be discussed.
There are two reasons to do it that way:
1. Replacing data objects with funcitons-wrappers do not make any sense,
because it is not called finaly. It just leads to a strange error if it 
indexed somewhere.
2. Strange error messages appear in bg fibers logs sometimes because of 
reason 1.
>
>> +    end
>> +    log.info('Old replicaset and replica objects are outdated.')
>> +end
>> +
>>   --
>>   -- Calculate for each replicaset its etalon bucket count.
>>   -- Iterative algorithm is used to learn the best balance in a
>> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
>> index a143070..a8f6bbc 100644
>> --- a/vshard/router/init.lua
>> +++ b/vshard/router/init.lua
>> @@ -479,12 +493,13 @@ local function router_cfg(cfg)
>>       else
>>           log.info('Starting router reconfiguration')
>>       end
>> -    local new_replicasets = lreplicaset.buildall(cfg, M.replicasets)
>> +    local new_replicasets = lreplicaset.buildall(cfg)
>>       local total_bucket_count = cfg.bucket_count
>>       local collect_lua_garbage = cfg.collect_lua_garbage
>> -    lcfg.remove_non_box_options(cfg)
>> +    local box_cfg = table.deepcopy(cfg)
>
> 12. As I remember, I asked to use table.copy when
> possible. And this case looks appropriate.
ok
>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
>> index 052e94f..bf560e6 100644
>> --- a/vshard/storage/init.lua
>> +++ b/vshard/storage/init.lua
>> @@ -2,20 +2,34 @@ local log = require('log')
>>   local luri = require('uri')
>>   local lfiber = require('fiber')
>>   local netbox = require('net.box') -- for net.box:self()
>> +local trigger = require('internal.trigger')
>> +
>> +local MODULE_INTERNALS = '__module_vshard_storage'
>> +-- Reload requirements, in case this module is reloaded manually.
>> +if rawget(_G, MODULE_INTERNALS) then
>> +    local vshard_modules = {
>> +        'vshard.consts', 'vshard.error', 'vshard.cfg',
>> +        'vshard.replicaset', 'vshard.util',
>> +    }
>> +    for _, module in pairs(vshard_modules) do
>> +        package.loaded[module] = nil
>> +    end
>> +end
>>   local consts = require('vshard.consts')
>>   local lerror = require('vshard.error')
>> -local util = require('vshard.util')
>>   local lcfg = require('vshard.cfg')
>>   local lreplicaset = require('vshard.replicaset')
>> -local trigger = require('internal.trigger')
>> +local util = require('vshard.util')
>>   -local M = rawget(_G, '__module_vshard_storage')
>> +local M = rawget(_G, MODULE_INTERNALS)
>>   if not M then
>>       --
>>       -- The module is loaded for the first time.
>>       --
>>       M = {
>>           ---------------- Common module attributes ----------------
>> +        -- The last passed configuration.
>> +        current_cfg = nil,
>
> 13. Please, add the same assignment to the router module
> initialization.
added