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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 7259 bytes --]

Hi Alexandr!

Thanks for the review! I have fixed all nits and rewrite the commit message.

Fix the return value of box.info:memory(). It used to return the same 
table as the box.info(). | box.info.memory() = box.info[“memory”]() | 
box.info.memory() -> 
getmetatable(box.info.memory).__call(box.info.memory) | 
box.info:memory() = box.info[“memory”](box.info) | box.info:memory() -> 
getmetatable(box.info...).__call(box.info.., box.info) See 
https://www.lua.org/manual/5.1/manual.html#2.8.An extra argument on Lua 
stack in the second case is box.info table with __serialize metamethod. 
When all internal functions are over, memory table is pushed on the 
stack. When console prints the output (converts lua table to yaml format 
and prints) __serialize metamethod of box.info is called. __serilize 
metamethod of box.info shows only some predefined fields and completely 
ignores memory table. To avoid this call, patch pushes an extra empty 
table on Lua stack. When console calls ymal.serializer to print lua 
table, box.info will be ignored. Closes #4688 
<https://github.com/tarantool/tarantool/issues/4688>

09.07.2020 4:08, Alexander Turenko пишет:
> 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
fixed
>
>>      
>>      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.
fixed
>
>>      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.
fixed
>>      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:
fixed
>
>   | 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
Changed
> 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.
Fixed
>> 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().
added an epmty line.
>
> 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, <...>)
New test:
  --
  -- gh-4688: box.info:memory() displayed full content of box.info
  --
  local tap = require('tap')
-local test = tap.test("Tarantool 4688")
-test:plan(1)
~
  box.cfg()
~
-a = box.info.memory()
-b = box.info:memory()
+local test = tap.test('gh-4688-box.info:memory-wrong-result')
+test:plan(1)
+
+local a = box.info.memory()
+local b = box.info:memory()
+
+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, "box.info:memory coincide with 
box.info.memory")
~
-test:is(table.concat(a), table.concat(b), "box.info:memory")
-os.exit(0)
+os.exit(test:check() and 0 or 1)
>
> 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

[-- Attachment #2: Type: text/html, Size: 17710 bytes --]