From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D10A745C304 for ; Wed, 2 Dec 2020 18:25:24 +0300 (MSK) References: <20201115235416.72858-1-roman.habibov@tarantool.org> <39A73765-A387-4EA2-83DA-4B80534AAAF0@tarantool.org> <2046B5A1-96D9-48D5-9F17-7AC1EAE83496@tarantool.org> <271345E1-6C02-40D2-ADC3-50C96F4BCA90@tarantool.org> <68080f1b-a50d-3e7b-cb3a-7b8e8d6e437b@tarantool.org> <5F726C10-5FE5-4494-B08A-5657AD44C894@tarantool.org> From: roman Message-ID: <1817a3ee-fd68-25dd-58d2-9a75fd747f5b@tarantool.org> Date: Wed, 2 Dec 2020 18:25:21 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: kyukhin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for LGTM. Kirill, you can push it. On 02.12.2020 12:17, Serge Petrenko wrote: > > 02.12.2020 03:16, Roman Khabibov пишет: >> Hi! Thanks for the review. >>> On Dec 1, 2020, at 12:58, Serge Petrenko >>> 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 >>>> 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. >>>