From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Alex Khatskevich <avkhatskevich@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce storage reload evolution Date: Fri, 20 Jul 2018 17:15:22 +0300 [thread overview] Message-ID: <d7eec7a3-3531-e6f4-c367-a3191e1bcb7e@tarantool.org> (raw) In-Reply-To: <76cb41bd-cce4-8483-9222-39176a146ce4@tarantool.org> Thanks for the fixes! See 4 comments below. >> >>> diff --git a/test/reload_evolution/storage_1_a.lua b/test/reload_evolution/storage_1_a.lua >>> new file mode 100755 >>> index 0000000..3e03f8f >>> --- /dev/null >>> +++ b/test/reload_evolution/storage_1_a.lua >>> @@ -0,0 +1,144 @@ >>> +#!/usr/bin/env tarantool >> >> 11. Just use a symbolic link to the existing storage_1_a. Same >> for other storages. >> >>> diff --git a/test/reload_evolution/suite.ini b/test/reload_evolution/suite.ini >>> new file mode 100644 >>> index 0000000..bb5435b >>> --- /dev/null >>> +++ b/test/reload_evolution/suite.ini >> >> 12. You do not need a new test suite for one storage test. Please, >> put it into test/storage/ > 11-12: > I have modified storage_1_a to change luapath. > It is important to change path here, because the default Vshard initialization is performed here. 1. Then please explain why do you need in storage_1_a.lua * two "require('fio')"; * '-- Check if we are running under test-run'; * 'names' table, 'replicasets' table; * 'box.once("testapp:schema:1", function()'; * ...many other things. If you do not need them, then please, cleanup your storage_1_a.lua and extract wait_rebalancer_state into test utils since it is now duplicated in storage/ and reload_evolution/ tests. >> >>> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua >>> index bf560e6..1740c98 100644 >>> --- a/vshard/storage/init.lua >>> +++ b/vshard/storage/init.lua >>> @@ -105,6 +110,11 @@ if not M then >>> -- a destination replicaset must drop already received >>> -- data. >>> rebalancer_sending_bucket = 0, >>> + >>> + ------------------------- Reload ------------------------- >>> + -- Version of the loaded module. This number is used on >>> + -- reload to determine which upgrade scripts to run. >>> + reload_evolution_version = reload_evolution.version, >> >> 13. Use 'version' name. > But it is not a version. Version is a git tag. > We already have module_version and git tag. To make everything clear I would like to > name the variable in that or similar way. 2. 'git tag' works only when you have git. But when you install vshard from a package, the system can have no git. Moreover, it contradicts with tarantool versioning, that exposes the actual version via _schema and box.info. So please, use real version for this member and name it 'version'. You can find the code to extract, store and compare versions in box/lua/upgrade.lua. >> >>> } >>> end >>> diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua >>> new file mode 100644 >>> index 0000000..cfac888 >>> --- /dev/null >>> +++ b/vshard/storage/reload_evolution.lua >>> @@ -0,0 +1,58 @@ >>> +-- >>> +-- This module is used to upgrade the vshard.storage on the fly. >>> +-- It updates internal Lua structures in case they are changed >>> +-- in a commit. >>> +-- >>> +local log = require('log') >>> + >>> +-- >>> +-- Array of upgrade functions. >>> +-- magrations[version] = function which upgrades module version >> >> 14. Typo: magrations. > fixed >> >>> +-- from `version` to `version + 1`. >> >> 15. Not +1. I think, we should use real version numbers: >> 0.1.1, 0.1.2, etc similar to tarantool. > Disagree. That would make us to > 1. Tag commit each time M is changed 3. It is not the same as git tag. We can add here a new version and tag it later, like it is done in tarantool, when we use the new version everywhere in the new code, but release it when ready. For example, now vshard has version 1.2.3. We change M and set version in evolution of M in the 1.2.4 constant. When 1.2.4 is ready, we do git tag 1.2.4. > 2. Maintain identical git tags and reload_evolution_version names. > In the introduced all you need to do is just to append callback to the > `migrations` array. 4. You are trying to make you life easier, but do not think about users. It is not useful when you are not able to get the actual version, because you can not use git, or you see the strange versions like 1, 2, 3, 4 ... Nobody uses natural numbers as versions. It is weird. I think, we should consult Kostja N. for this though.
next prev parent reply other threads:[~2018-07-20 14:15 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-18 17:47 [tarantool-patches] [PATCH 0/3] vshard reload mechanism AKhatskevich 2018-07-18 17:47 ` [tarantool-patches] [PATCH 1/3] Add test on error during reconfigure AKhatskevich 2018-07-19 15:14 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-19 20:33 ` Alex Khatskevich 2018-07-20 11:34 ` Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy 2018-07-18 17:47 ` [tarantool-patches] [PATCH 2/3] Complete module reload AKhatskevich 2018-07-19 15:14 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-19 20:32 ` Alex Khatskevich 2018-07-20 11:34 ` Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy 2018-07-18 17:47 ` [tarantool-patches] [PATCH 3/3] Introduce storage reload evolution AKhatskevich 2018-07-19 15:14 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-20 11:32 ` Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy [this message] 2018-07-20 23:58 ` Alex Khatskevich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d7eec7a3-3531-e6f4-c367-a3191e1bcb7e@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=avkhatskevich@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 3/3] Introduce storage reload evolution' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox