From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 5D12E469710 for ; Wed, 27 May 2020 15:59:33 +0300 (MSK) From: Oleg Babin References: <20200527111406.145422-1-gorcunov@gmail.com> <20200527111406.145422-10-gorcunov@gmail.com> Message-ID: <92c58753-30f0-53b9-84b4-7dc81129452b@tarantool.org> Date: Wed, 27 May 2020 15:59:29 +0300 MIME-Version: 1.0 In-Reply-To: <20200527111406.145422-10-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 09/10] lua/log: allow to configure logging without a box List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Thanks for your patch. See several comments below. It's just nitpicks, in some places I only write how it should be changed to respect Tarantool lua style guide. On 27/05/2020 14:14, Cyrill Gorcunov wrote: > In the commit we allow to configure logger without > calling box.cfg{}. > > Fixes #689 > > @TarantoolBot document > Title: Module log > > Module log allows to setup logger early without calling ``box.cfg()``. > Configuration parameters are same as in ``box.cfg()`` call. > > Example > ``` >> log = require('log') >> log.cfg({log='filename', log_format='plain', log_level=6}) > ... >> log.cfg({log_format='json', log_level=5}) > ``` > > Signed-off-by: Cyrill Gorcunov > --- > src/box/lua/load_cfg.lua | 13 +++-- > src/lua/log.lua | 117 +++++++++++++++++++++++++++++++++++++-- > 2 files changed, 119 insertions(+), 11 deletions(-) > > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index 9aef12840..00e424156 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -471,11 +471,13 @@ end > local function apply_default_modules_cfg(cfg) > -- > -- logging > - for k,v in pairs(log.box_api.cfg) do > - if cfg[k] == nil then > - cfg[k] = v > - end > - end > + log.box_api.cfg_apply_default(cfg) > +end > + > +local function update_modules_cfg(cfg) > + -- > + -- logging > + log.box_api.cfg_update(cfg) > end > > -- Return true if two configurations are equivalent. > @@ -597,6 +599,7 @@ local function load_cfg(cfg) > end > end > end > + update_modules_cfg(cfg) > if not box.cfg.read_only and not box.cfg.replication then > box.schema.upgrade{auto = true} > end > diff --git a/src/lua/log.lua b/src/lua/log.lua > index 25c919412..c14769e84 100644 > --- a/src/lua/log.lua > +++ b/src/lua/log.lua > @@ -97,6 +97,14 @@ local fmt_str2num = { > ["json"] = ffi.C.SF_JSON, > } > > +local function fmt_list() > + local keyset = {} > + for k,_ in pairs(fmt_str2num) do > + keyset[#keyset+1] = k nit: spaces around `+` are required by tarantool lua style guide (https://www.tarantool.io/en/doc/2.2/dev_guide/lua_style_guide) > + end > + return table.concat(keyset,',') nit: space between a comma and argument required is required by lua style guide > +end > + > -- > -- Default options. The keys are part of > -- user API, so change with caution. > @@ -162,18 +170,17 @@ local function log_rotate() > end > > local function log_level(level) > + if type(level) ~= 'number' then > + error("log: 'level' (or 'log_level' option) expects a number") > + end > ffi.C.say_set_log_level(level) > rawset(log_cfg, 'log_level', level) > end > > local function log_format(name) > if not fmt_str2num[name] then > - local keyset = {} > - for k,_ in pairs(fmt_str2num) do > - keyset[#keyset+1] = k > - end > local m = "log_format: expected %s" > - error(m:format(table.concat(keyset,','))) > + error(m:format(fmt_list())) > end > > if fmt_str2num[name] == ffi.C.SF_JSON then > @@ -194,6 +201,100 @@ local function log_pid() > return tonumber(ffi.C.log_pid) > end > > +local cfg_params_once = { > + 'log', > + 'log_nonblock', > +} > + > +-- > +-- Setup dynamic parameters. > +local function dynamic_cfg(cfg) > + > + for _,k in pairs(cfg_params_once) do nit: `_,k` -> `_, k` > + if cfg[k] ~= nil and cfg[k] ~= log_cfg[k] then > + local m = "log: '%s' can't be set dynamically" > + error(m:format(k)) > + end > + end > + > + if cfg.log_level ~= nil then > + log_level(cfg.log_level) > + end > + > + if cfg.log_format ~= nil then > + log_format(cfg.log_format) > + end > +end > + > +-- > +-- Load or reload configuration via log.cfg({}) > +-- > +-- The syntax is the same as for box.cfg({}). > +local function load_cfg(oldcfg, cfg) > + cfg = cfg or {} > + > + if cfg.log_format ~= nil then > + if fmt_str2num[cfg.log_format] == nil then > + local m = "log: 'log_format' must be %s" > + error(m:format(fmt_list())) > + end > + end > + > + if cfg.log_level ~= nil then > + if type(cfg.log_level) ~= 'number' then > + error("log: 'log_level' option must be a number") > + end > + end > + > + if cfg.log_nonblock ~=nil then nit: `~=nil` -> `~= nil` > + if type(cfg.log_nonblock) ~= 'boolean' then > + error("log: 'log_nonblock' option must be 'true' or 'false'") > + end > + end > + > + if ffi.C.say_logger_initialized() == true then > + return dynamic_cfg(cfg) > + end > + > + cfg.log_level = cfg.log_level or log_cfg.log_level > + cfg.log_format = cfg.log_format or log_cfg.log_format > + cfg.log_nonblock = cfg.log_nonblock or (log_cfg.log_nonblock or false) > + > + -- > + -- We never allow confgure the logger in background > + -- mode since we don't know how the box will be configured > + -- later. > + ffi.C.say_logger_init(cfg.log, cfg.log_level, > + cfg.log_nonblock, cfg.log_format, 0) > + > + -- > + -- Update log_cfg vars to show them in module > + -- configuration output. > + rawset(log_cfg, 'log', cfg.log) > + rawset(log_cfg, 'log_level', cfg.log_level) > + rawset(log_cfg, 'log_nonblock', cfg.log_nonblock) > + rawset(log_cfg, 'log_format', cfg.log_format) > +end > + > +-- > +-- Apply defaut config to the box module > +local function box_cfg_apply_default(box_cfg) > + for k,v in pairs(log_cfg) do > + if box_cfg[k] == nil then > + box_cfg[k] = v > + end > + end > +end > + > +-- > +-- Reflect the changes made by box.cfg interface > +local function box_cfg_update(box_cfg) > + rawset(log_cfg, 'log', box_cfg.log) > + rawset(log_cfg, 'log_level', box_cfg.log_level) > + rawset(log_cfg, 'log_nonblock', box_cfg.log_nonblock) > + rawset(log_cfg, 'log_format', box_cfg.log_format) > +end > + > local compat_warning_said = false > local compat_v16 = { > logger_pid = function() > @@ -215,17 +316,21 @@ local log ={ > pid = log_pid, > level = log_level, > log_format = log_format, > + cfg = setmetatable(log_cfg, { > + __call = load_cfg, > + }), > -- > -- Internal API to box module, not for users, > -- names can be changed. > box_api = { > - cfg = log_cfg, > set_log_level = function() > log_level(box.cfg.log_level) > end, > set_log_format = function() > log_format(box.cfg.log_format) > end, > + cfg_apply_default = box_cfg_apply_default, > + cfg_update = box_cfg_update, > }, > } > >