Tarantool development patches archive
 help / color / mirror / Atom feed
From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 6/9] cfg: introduce 'deprecated option' feature
Date: Thu, 11 Feb 2021 09:50:48 +0300
Message-ID: <2955c6ee-a9ad-4667-b8bc-947ab2f3297a@tarantool.org> (raw)
In-Reply-To: <1b189f47-1901-c55f-e103-80222c7730ff@tarantool.org>

Thanks for your fixes. LGTM!

On 11/02/2021 01:34, Vladislav Shpilevoy wrote:
> Thanks for the review!
>
> On 10.02.2021 09:59, Oleg Babin wrote:
>> Thanks for your patch!
>>
>> Is it possible to extend log message to "Option is deprecated and has no effect anymore"?
> Good idea. See the diff in this commit.
>
> ====================
> diff --git a/vshard/cfg.lua b/vshard/cfg.lua
> index 28c3400..f7d5dbc 100644
> --- a/vshard/cfg.lua
> +++ b/vshard/cfg.lua
> @@ -61,7 +61,13 @@ local function validate_config(config, template, check_arg)
>           local expected_type = template_value.type
>           if template_value.is_deprecated then
>               if value ~= nil then
> -                log.warn('Option "%s" is deprecated', name)
> +                local reason = template_value.reason
> +                if reason then
> +                    reason = '. '..reason
> +                else
> +                    reason = ''
> +                end
> +                log.warn('Option "%s" is deprecated'..reason, name)
>               end
>           elseif value == nil then
>               if not template_value.is_optional then
> ====================
>
> And in the next commit:
>
> ====================
> diff --git a/vshard/cfg.lua b/vshard/cfg.lua
> index f7dd4c1..63d5414 100644
> --- a/vshard/cfg.lua
> +++ b/vshard/cfg.lua
> @@ -252,6 +252,7 @@ local cfg_template = {
>       },
>       collect_bucket_garbage_interval = {
>           name = 'Garbage bucket collect interval', is_deprecated = true,
> +        reason = 'Has no effect anymore'
>       },
>       collect_lua_garbage = {
>           type = 'boolean', name = 'Garbage Lua collect necessity',
>
> ====================
>
>> Also for some options could be useful: "Option is deprecated, use ... instead" (e.g. for "weights").
> With the updated version I can specify any 'reason'. Such as
> 'has no affect', 'use ... instead', etc.
>
>> Seems it should be more configurable and gives some hint for user to do.
>>
>>
>> On 10/02/2021 02:46, Vladislav Shpilevoy wrote:
>>> Some options in vshard are going to be eventually deprecated. For
>>> instance, 'weigts' will be renamed, 'collect_lua_garbage' may be
>> typo: weigts -> weights
> Fixed. See the full new patch below.
>
> ====================
>      cfg: introduce 'deprecated option' feature
>      
>      Some options in vshard are going to be eventually deprecated. For
>      instance, 'weights' will be renamed, 'collect_lua_garbage' may be
>      deleted since it appears not to be so useful, 'sync_timeout' is
>      totally unnecessary since any 'sync' can take a timeout per-call.
>      
>      But the patch is motivated by 'collect_bucket_garbage_interval'
>      which is going to become unused in the new GC algorithm.
>      
>      New GC will be reactive instead of proactive. Instead of periodic
>      polling of _bucket space it will react on needed events
>      immediately. This will make the 'collect interval' unused.
>      
>      The option will be deprecated and eventually in some far future
>      release its usage will lead to an error.
>      
>      Needed for #147
>
> diff --git a/vshard/cfg.lua b/vshard/cfg.lua
> index 1ef1899..f7d5dbc 100644
> --- a/vshard/cfg.lua
> +++ b/vshard/cfg.lua
> @@ -59,7 +59,17 @@ local function validate_config(config, template, check_arg)
>           local value = config[key]
>           local name = template_value.name
>           local expected_type = template_value.type
> -        if value == nil then
> +        if template_value.is_deprecated then
> +            if value ~= nil then
> +                local reason = template_value.reason
> +                if reason then
> +                    reason = '. '..reason
> +                else
> +                    reason = ''
> +                end
> +                log.warn('Option "%s" is deprecated'..reason, name)
> +            end
> +        elseif value == nil then
>               if not template_value.is_optional then
>                   error(string.format('%s must be specified', name))
>               else
>
> ====================

  reply	other threads:[~2021-02-11  6:51 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-09 23:46 [Tarantool-patches] [PATCH 0/9] VShard Map-Reduce, part 1, preparations Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 1/9] rlist: move rlist to a new module Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches
2021-02-11  6:50     ` Oleg Babin via Tarantool-patches
2021-02-12  0:09       ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 2/9] Use fiber.clock() instead of .time() everywhere Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches
2021-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 3/9] test: introduce a helper to wait for bucket GC Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:57   ` Oleg Babin via Tarantool-patches
2021-02-10 22:33     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 4/9] storage: bucket_recv() should check rs lock Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 5/9] util: introduce yielding table functions Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-10 22:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 6/9] cfg: introduce 'deprecated option' feature Vladislav Shpilevoy via Tarantool-patches
2021-02-10  8:59   ` Oleg Babin via Tarantool-patches
2021-02-10 22:34     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches [this message]
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 7/9] gc: introduce reactive garbage collector Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:00   ` Oleg Babin via Tarantool-patches
2021-02-10 22:35     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:50       ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 8/9] recovery: introduce reactive recovery Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:00   ` Oleg Babin via Tarantool-patches
2021-02-09 23:46 ` [Tarantool-patches] [PATCH 9/9] util: introduce binary heap data structure Vladislav Shpilevoy via Tarantool-patches
2021-02-10  9:01   ` Oleg Babin via Tarantool-patches
2021-02-10 22:36     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-11  6:51       ` Oleg Babin via Tarantool-patches
2021-02-12  0:09         ` Vladislav Shpilevoy via Tarantool-patches
2021-03-05 22:03   ` Vladislav Shpilevoy via Tarantool-patches
2021-02-09 23:51 ` [Tarantool-patches] [PATCH 0/9] VShard Map-Reduce, part 1, preparations Vladislav Shpilevoy via Tarantool-patches
2021-02-12 11:02   ` Oleg Babin via Tarantool-patches

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=2955c6ee-a9ad-4667-b8bc-947ab2f3297a@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git