[tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start()
Roman Khabibov
roman.habibov at tarantool.org
Fri Apr 12 12:29:42 MSK 2019
Hi! Thanks for the review.
> On Apr 11, 2019, at 5:57 PM, Alexander Turenko <alexander.turenko at tarantool.org> wrote:
>
> The behaviour is almost okay, but you need to work on wording of the
> commit message to make it useful for a reader. Feel free to ask more if
> you have questions.
>
> See comments below.
>
> WBR, Alexander Turenko.
>
>> commit 4cdf8e79bb530288098fb30e5014498fc0ad4262
>> Author: Roman Khabibov <roman.habibov at tarantool.org>
>> Date: Mon Mar 25 20:52:05 2019 +0300
>>
>> tarantoolctl: error when box.cfg didn't called
>
> Cite from our developer guidelines [1]:
>
>> 6. Use the imperative mood in the subject line. A properly formed Git
>> commit subject line should always be able to complete the following
>> sentence: “If applied, this commit will /your subject line here/”.
>
> [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message
>
> Also you construct passive voice wrongly, it should be 'to be + [not] + Ved'.
>
>>
>> There was assumpted uninitialized metamethods of box.cfg{} if user
>> did't call it in his instance.
>
> This sentence raises more questions then give answers. What is
> 'uninitialized metamethods'? Whether 'of box.cfg{}' means result of the
> call or a new value of the 'cfg' field of the 'box' table?
>
> Also I doubt 'There was assumpted' is valid phrase.
tarantoolctl: raise error when box.cfg isn't called
There was no check whether box.cfg{} was initialized in an
instance. If so, an error should be raised.
>>
>> Closes #3953
>>
>> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
>> index 47fcf895f..396fa7957 100755
>> --- a/extra/dist/tarantoolctl.in
>> +++ b/extra/dist/tarantoolctl.in
>> @@ -547,7 +547,11 @@ local function start()
>> end
>> os.exit(1)
>> end
>> - local old_call = getmetatable(box.cfg).__call
>> + local m_table = getmetatable(box.cfg)
>> + if m_table == nil then
>
> Cite from our Lua style guide [2]:
>
>> `*_mt` and `*_methods` defines metatable and methods table
>
> Suggested to name it box_cfg_mt.
+ local box_cfg_mt = getmetatable(box.cfg)
> [2]: https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/
>
>> + error("Plaese, call box.cfg{} within your instance")
>
> Typo: 'Plaese'.
>
> I would change wording to just state what is wrong, e.g.: 'box.cfg() is
> not called in an instance file'.
>
> See also how errors are reported in other places: log.error() and
> os.exit(1). I guess it does not matter much from a behaviour point of
> view, but it would be good to report errors in one way with surrounding
> code.
>
> (BTW, found one behaviour difference: error() reports a line number when
> called with one argument or non-zero second argument.)
>
> Please, install tarantool and tarantoolctl and check the behaviour:
> messages and exit codes. Currently it looks so:
>
> # tarantoolctl start foo
> Starting instance foo...
> Hello, World!
> /usr/bin/tarantoolctl:552: Plaese, call box.cfg{} within your instance
> # echo $?
> 1
>
> It is okay except file:line part, but anyway I suggest you to use
> log.error() and os.exit(1).
+ if box_cfg_mt == nil then
+ log.error('box.cfg() is not called in an instance file')
+ os.exit(1)
>
>> if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
>> cfg.pid_file = old_cfg.pid_file
>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index db046e03f..428b074e2 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -165,10 +165,13 @@ do
>> local dir = fio.tempdir()
>> local code = [[ box.cfg{memtx_memory = 104857600} ]]
>> create_script(dir, 'script.lua', code)
>> + create_script(dir, 'init.lua', [[ print('Hi!') ]])
>>
>> local status, err = pcall(function()
>> test:test("basic test", function(test_i)
>> - test_i:plan(16)
>> + test_i:plan(18)
>> + check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...",
>> + "Plaese, call box.cfg{} within your instance")
>> check_ok(test_i, dir, 'start', 'script', 0, nil, "Starting instance")
>> tctl_wait_start(dir, 'script')
>> check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
>>
>>
>
> I propose to make the code of the test a bit more uniform and use a
> meaningful instance file name:
>
> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 428b074e2..7d7c63371 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -163,15 +163,14 @@ test:plan(7)
> -- must be stopped afterwards
> do
> local dir = fio.tempdir()
> - local code = [[ box.cfg{memtx_memory = 104857600} ]]
> - create_script(dir, 'script.lua', code)
> - create_script(dir, 'init.lua', [[ print('Hi!') ]])
> + create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
> + create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
>
> local status, err = pcall(function()
> test:test("basic test", function(test_i)
> test_i:plan(18)
> - check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...",
> - "Plaese, call box.cfg{} within your instance")
> + check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
> + "box.cfg() is not called in an instance file")
> check_ok(test_i, dir, 'start', 'script', 0, nil, "Starting instance")
> tctl_wait_start(dir, 'script')
> check_ok(test_i, dir, 'status', 'script', 0, nil, "is running”)
- local code = [[ box.cfg{memtx_memory = 104857600} ]]
- create_script(dir, 'script.lua', code)
+ create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
+ create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
local status, err = pcall(function()
test:test("basic test", function(test_i)
- test_i:plan(16)
+ test_i:plan(18)
+ check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
+ "box.cfg() is not called in an instance file”)
commit 834545cc83965f6d612fcccd39dda03074e7f227
Author: Roman Khabibov <roman.habibov at tarantool.org>
Date: Mon Mar 25 20:52:05 2019 +0300
tarantoolctl: raise error when box.cfg isn't called
There was no check whether box.cfg{} was initialized in an
instance. If so, an error should be raised.
Closes #3953
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 47fcf895f..37ded5333 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -547,8 +547,13 @@ local function start()
end
os.exit(1)
end
- local old_call = getmetatable(box.cfg).__call
- getmetatable(box.cfg).__call = function(old_cfg, cfg)
+ local box_cfg_mt = getmetatable(box.cfg)
+ if box_cfg_mt == nil then
+ log.error('box.cfg() is not called in an instance file')
+ os.exit(1)
+ end
+ local old_call = box_cfg_mt.__call
+ box_cfg_mt.__call = function(old_cfg, cfg)
if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
cfg.pid_file = old_cfg.pid_file
end
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index db046e03f..7d7c63371 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -163,12 +163,14 @@ test:plan(7)
-- must be stopped afterwards
do
local dir = fio.tempdir()
- local code = [[ box.cfg{memtx_memory = 104857600} ]]
- create_script(dir, 'script.lua', code)
+ create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]])
+ create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]])
local status, err = pcall(function()
test:test("basic test", function(test_i)
- test_i:plan(16)
+ test_i:plan(18)
+ check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance",
+ "box.cfg() is not called in an instance file")
check_ok(test_i, dir, 'start', 'script', 0, nil, "Starting instance")
tctl_wait_start(dir, 'script')
check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")
More information about the Tarantool-patches
mailing list