Re: [Tarantool-patches] [PATCH v2 vshard 1/2] storage: fix reload applying migration twice

[Oleg Babin <olegrok@tarantool.org>]

Hi! Thanks for your patch. LGTM.

On 06/08/2020 01:15, Vladislav Shpilevoy wrote:
> Reload evolution is a subsystem for storage hot code reload. When
> code is reloaded, it applies migration scripts one by one. But
> there was a bug - the last migration script was applied even when
> code version didn't really change.
>
> The bug didn't affect anything, since so far the reload evolution
> was not used for anything and had just one dummy migration script
> doing nothing.
>
> Now a first migration script is going to be added, so the bug is
> fixed before that.
>
> Needed for #237
> ---
>   test/reload_evolution/storage.result   | 6 +++++-
>   test/reload_evolution/storage.test.lua | 6 +++++-
>   vshard/storage/reload_evolution.lua    | 2 +-
>   3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result
> index ebf4fdc..dde0a1f 100644
> --- a/test/reload_evolution/storage.result
> +++ b/test/reload_evolution/storage.result
> @@ -86,7 +86,11 @@ package.loaded['vshard.storage'] = nil
>   vshard.storage = require("vshard.storage")
>   ---
>   ...
> -test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil
> +-- Should be nil. Because there was a bug that reload always reapplied the last
> +-- migration callback. When it was fixed, the last callback wasn't called twice.
> +-- But since the callback was only one, now nothing is called, and nothing is
> +-- logged.
> +test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil
>   ---
>   - true
>   ...
> diff --git a/test/reload_evolution/storage.test.lua b/test/reload_evolution/storage.test.lua
> index 56c1693..7858112 100644
> --- a/test/reload_evolution/storage.test.lua
> +++ b/test/reload_evolution/storage.test.lua
> @@ -35,7 +35,11 @@ box.space.test:insert({42, bucket_id_to_move})
>   package.path = original_package_path
>   package.loaded['vshard.storage'] = nil
>   vshard.storage = require("vshard.storage")
> -test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil
> +-- Should be nil. Because there was a bug that reload always reapplied the last
> +-- migration callback. When it was fixed, the last callback wasn't called twice.
> +-- But since the callback was only one, now nothing is called, and nothing is
> +-- logged.
> +test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil
>   vshard.storage.internal.reload_version
>   -- Make sure storage operates well.
>   vshard.storage.bucket_force_drop(2000)
> diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua
> index 5d09b11..1abc9e2 100644
> --- a/vshard/storage/reload_evolution.lua
> +++ b/vshard/storage/reload_evolution.lua
> @@ -33,7 +33,7 @@ local function upgrade(M)
>           log.error(err_msg)
>           error(err_msg)
>       end
> -    for i = start_version, #migrations  do
> +    for i = start_version + 1, #migrations do
>           local ok, err = pcall(migrations[i], M)
>           if ok then
>               log.info('vshard.storage.reload_evolution: upgraded to %d version',