Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update

[Serge Petrenko <sergepetrenko@tarantool.org>]

02.12.2020 03:16, Roman Khabibov пишет:
> Hi! Thanks for the review.
>
>> On Dec 1, 2020, at 12:58, Serge Petrenko <sergepetrenko@tarantool.org> wrote:
>>
>>
>> 30.11.2020 16:43, Roman Khabibov пишет:
>>> Thanks.
>>>
>>> Serge, could you, please, look through the patch?
>>
>> Hi! Thanks for the patch!
>>
>> Since you're working on this instead of Sergey now, you may add yourself to
>>
>> the Co-developed-by field in the commit message, like it is done here:
>>
>> https://github.com/tarantool/tarantool/commit/cfccfd449c890c18615185ba4895d9081e50c318
>>
>>
>> Please see 3 more comments below.
>>
>>
>>> commit 8b3265c1599772f5a85e47ed1a8232571ec23f8d
>>> Author: Sergey Voinov <sergeiv@tarantool.org>
>>> Date:   Wed Dec 11 17:28:39 2019 +0300
>>>
>>>      box: check schema version after tarantool update
>>>           Check schema version (stored in box.space._schema) on start and
>>>      print a warning if it doesn't match last available schema version.
>>>      It is needed because some users forget to call
>>>      box.schema.upgrade() after Tarantool update and get stuck with an
>>>      old schema version until they encounter some hard to debug
>>>      problems.
>>>           Closes #4574
>>>
>>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>>> index 76e2e92c2..451247dcf 100644
>>> --- a/src/box/lua/load_cfg.lua
>>> +++ b/src/box/lua/load_cfg.lua
>>> @@ -702,6 +702,22 @@ local function load_cfg(cfg)
>>>       box_configured = nil
>>>         box_is_configured = true
>>> +
>>> +    -- Check if schema version matches Tarantool version
>>> +    -- and print warning if it's not (in case user forgot to call box.schema.upgrade())
>>> +    local version = box.space._schema:get{'version'}
>>
>> 1. Version unused. You get schema version in `schema_needs_upgrade()` anyway.
>>
>>     Also you may omit testing for nil here. You may just test schema version
>>     inside `schema_needs_upgrade()` and simply return false, if it is nil.
>>
>>
>>     You'll also need to update test/box/stat.result after this is done.
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 76e2e92c2..770442052 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -702,6 +702,18 @@ local function load_cfg(cfg)
>       box_configured = nil
>   
>       box_is_configured = true
> +
> +    -- Check if schema version matches Tarantool version and print
> +    -- warning if it's not (in case user forgot to call
> +    -- box.schema.upgrade()).
> +    local needs, schema_version_str = private.schema_needs_upgrade()
> +    if needs then
> +        local msg = string.format(
> +            'Your schema version is %s while Tarantool %s requires a more'..
> +            ' recent schema version. Please, consider using box.'..
> +            'schema.upgrade().', schema_version_str, box.info.version)
> +        log.warn(msg)
> +    end
>   end
>   box.cfg = locked(load_cfg)
>
>>> +    if version ~= nil then
>>> +        local needs, schema_version_str = private.schema_needs_upgrade()
>>> +        local tarantool_version_str = box.info.version
>>> +        if needs then
>>> +            -- Print the warning
>>> +            local msg = string.format(
>>> +                'Your schema version is %s while Tarantool %s requires a more'..
>>> +                ' recent schema version. Please, consider using box.'..
>>> +                'schema.upgrade().', schema_version_str, tarantool_version_str)
>>> +            log.warn(msg)
>>> +        end
>>> +    end
>>>   end
>>>   box.cfg = locked(load_cfg)
>>>   
>>> diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
>>> index 56018b1a0..e806c9efe 100644
>>> --- a/test/box/cfg.test.lua
>>> +++ b/test/box/cfg.test.lua
>>> @@ -159,3 +159,21 @@ test_run:grep_log('cfg_tester7', 'set \'replication\' configuration option to',
>>>   test_run:grep_log('cfg_tester7', 'test%-cluster%-cookie', 1000)
>>>   test_run:cmd("stop server cfg_tester7")
>>>   test_run:cmd("cleanup server cfg_tester7")
>>> +
>>> +--
>>> +-- gh-4574: Check schema version after Tarantool update.
>>> +--
>>> +test_run:cmd('create server cfg_tester8 with script = "box/lua/cfg_test8.lua", workdir="sql/upgrade/2.1.0/"')
>>
>> 2. Can you reuse `cfg_test1.lua` here?
> No, I need to have "read_only = true”.


Oh, now I see. So that the instance doesn't auto-upgrade.

Thanks for the fixes! LGTM.


>>> +test_run:cmd("start server cfg_tester8")
>>> +--- Check that the warning is printed.
>>> +version_warning = "Please, consider using box.schema.upgrade()."
>>> +test_run:grep_log('cfg_tester8', version_warning, 1000) ~= nil
>>
>> 3. Better use `wait_log` instead of `grep_log`. It's not guaranteed that the
>>     server will print this message by the time you grep for it.
> Done.
>
>>
-- 
Serge Petrenko