Re: [Tarantool-patches] [PATCH 1/1] box: fixed box.info:memory()

[Alexander Turenko <alexander.turenko@tarantool.org>]

Pasted the actual patch to comment in-place.

> commit ad00de576ab0300a3e48be2cdda2bef5938eb40e
> Author: Olga Arkhangelskaia <arkholga@tarantool.org>
> Date:   Mon Jun 29 12:14:24 2020 +0300
> 
>     box: fix box.info:memory()
>     
>     Fix the output of box.info:memory(). It used to return the same table as the
>     box.info().

Nit: 72 symbols at max. Here and below.

Nit: I would say 'the return value', because 'the output' looks more

>     
>     Any box.info.xxx() is the same as box.info[“xxx”]().
>     E.g. box.info.memory() ->
>     getmetatable(box.info.memory).__call(box.info.memory)[1]

It is ambiguous: whether [1] is first element or an array or a reference
for the link below.

>     After __index and __call metamethods, the final function that fills xxx-table,
>     has the only argument - empty table to fill.
>     When box.info:xxx() is invoked it automatically passes one argument:
>     box.info[“xxx”](box.info). So the resulting call has 2 arguments on the stack.
>     box.info:xxx()->getmetatable(box.info.xxx).__call(box.info.xxx, box.info)

Nit: I would surround '->' with spaces for readability.

>     When function tries to fill box.info table - __call metamethod of box.info is
>     trigged.

It is not correct. It would be __newindex metamethod, but it is not
defined on the table. I guess you was misguided by a console output,
because of __serialize method. In fact <box.info> table will be filled
with 'cache', 'lua', 'data' and other box.info.memory() fields. You can
verify it youself:

 | tarantool> setmetatable(box.info:memory(), nil)
 | ---
 | - cache: 0
 |   lua: 1076262
 |   data: 37816
 |   index: 1097728
 |   net: 589824
 |   tx: 0
 |   version: 2.5.0-208-gcf6975793
 |   package: Tarantool
 | ...

Despite changes I requested above I appecitate the intention to clarify
the change.

>     
>     box.info.gc does not have this problem because of an extra table that is
>     created in the beginning of the bottom function. box.info.memory follows
>     the same way.
>     
>     [1] https://www.lua.org/manual/5.1/manual.html#2.8

Nit: Markdown provides '[1]: https://' syntax for reference style links,
but mayble there are others markups, where syntax is the same as above.
I don't know for sure. Personally I use markdown (but sometimes with
asciidoc titles) for texts with several simple markup elements like
hyperlinks. Many developers aware of this syntax.

>     
>     Closes 4688

Typo: no hash symbol.

> 
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index d0e553b1d..3d515ae9e 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -322,6 +322,7 @@ lbox_info_memory_call(struct lua_State *L)
>  	struct engine_memory_stat stat;
>  	engine_memory_stat(&stat);
>  
> +	lua_newtable(L);

Nit: Six same structured blocks are below. But after the change the
first one will differs. Please, add an empty line after lua_newtable().

BTW, we can use lua_createtable() to allocate a hashmap of necessary
size before inserting elements. It is to avoid resizing of the table
(don't know whether it is actual for small map sizes like 6).

The change itself is okay for me.

>  	lua_pushstring(L, "data");
>  	luaL_pushuint64(L, stat.data);
>  	lua_settable(L, -3);
> diff --git a/test/box-tap/gh-4688-box-info-memory.test.lua b/test/box-tap/gh-4688-box-info-memory.test.lua
> new file mode 100755
> index 000000000..63dcdab8f
> --- /dev/null
> +++ b/test/box-tap/gh-4688-box-info-memory.test.lua
> @@ -0,0 +1,15 @@
> +#!/usr/bin/env tarantool
> +--
> +-- gh-4688: box.info:memory() displayed full content of box.info
> +--
> +local tap = require('tap')
> +local test = tap.test("Tarantool 4688")

Nit: Single and double quotes are used without any system.

Nit: See how other top level test cases are named: `grep tap.test
test/*/gh-*`.

> +test:plan(1)
> +
> +box.cfg()
> +
> +a = box.info.memory()
> +b = box.info:memory()
> +
> +test:is(table.concat(a), table.concat(b), "box.info:memory")

First, 'lua' values likely will be different. Second, table.concat()
concatenates elements of an array like {'x', 'y', 'z'}. It is not for
maps (it just gives an empty string). The result is that the test passes
ever without the fix.

Most obvious way would be using of test:is_deeply(), but since 'lua'
field may vary, we can do one of the following ways:

 | a.lua = a.lua and '<stripped>' or nil
 | b.lua = b.lua and '<stripped>' or nil
 |
 | test:is_deeply(a, b, 'box.info:memory() is the same as box.info.memory()')

Or

 | local function get_keys(t)
 |     local keys = {}
 |     for k, v in pairs(t) do
 |         table.insert(keys, k)
 |     end
 |     return keys
 | end
 |
 | local keys_1 = get_keys(box.info.memory())
 | local keys_2 = get_keys(box.info:memory())
 | test:is_deeply(keys_1, keys_2, <...>)

Feel free to use any variant or provide your own.

> +os.exit(0)

Please, set exit code appropriately (see [1]).

[1]: https://github.com/tarantool/doc/issues/1004