<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Alexandr!</p>
<p>Thanks for the review! I have fixed all nits and rewrite the
commit message.</p>
<p><span style="box-sizing: inherit; user-select: text; cursor: text; color: rgb(36, 41, 46); font-family: Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; font-size: 11px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre-line; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;">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 </span><a class="link-button-component" href="https://www.lua.org/manual/5.1/manual.html#2.8." title="https://www.lua.org/manual/5.1/manual.html#2.8." style="box-sizing: inherit; user-select: text; cursor: pointer; color: var(--link-button-color); text-decoration: none; display: inline-flex; align-items: center; font-family: Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; font-size: 11px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre-line; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255);">https://www.lua.org/manual/5.1/manual.html#2.8.</a><span style="box-sizing: inherit; user-select: text; cursor: text; color: rgb(36, 41, 46); font-family: Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; font-size: 11px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre-line; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial;">
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 </span><a class="link-button-component" href="https://github.com/tarantool/tarantool/issues/4688" title="https://github.com/tarantool/tarantool/issues/4688" style="box-sizing: inherit; user-select: text; cursor: pointer; color: var(--link-button-color); text-decoration: none; display: inline-flex; align-items: center; font-family: Menlo, Monaco, Consolas, "Liberation Mono", "Courier New", monospace; font-size: 11px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre-line; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255);">#4688</a></p>
<div class="moz-cite-prefix">09.07.2020 4:08, Alexander Turenko
пишет:<br>
</div>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">Pasted the actual patch to comment in-place.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">commit ad00de576ab0300a3e48be2cdda2bef5938eb40e
Author: Olga Arkhangelskaia <a class="moz-txt-link-rfc2396E" href="mailto:arkholga@tarantool.org"><arkholga@tarantool.org></a>
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().
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: 72 symbols at max. Here and below.
Nit: I would say 'the return value', because 'the output' looks more</pre>
</blockquote>
fixed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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]
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
It is ambiguous: whether [1] is first element or an array or a reference
for the link below.</pre>
</blockquote>
fixed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: I would surround '->' with spaces for readability.
</pre>
</blockquote>
fixed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> When function tries to fill box.info table - __call metamethod of box.info is
trigged.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:</pre>
</blockquote>
fixed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
| 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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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] <a class="moz-txt-link-freetext" href="https://www.lua.org/manual/5.1/manual.html#2.8">https://www.lua.org/manual/5.1/manual.html#2.8</a></pre>
</blockquote>
</blockquote>
Changed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Closes 4688
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: no hash symbol.
</pre>
</blockquote>
Fixed<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
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);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Six same structured blocks are below. But after the change the
first one will differs. Please, add an empty line after lua_newtable().</pre>
</blockquote>
added an epmty line.<br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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")</pre>
</blockquote>
</blockquote>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
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-*`.
</pre>
</blockquote>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+test:plan(1)
+
+box.cfg()
+
+a = box.info.memory()
+b = box.info:memory()
+
+test:is(table.concat(a), table.concat(b), "box.info:memory")
</pre>
</blockquote>
</blockquote>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
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, <...>)</pre>
</blockquote>
New test:
<br>
--
<br>
-- gh-4688: box.info:memory() displayed full content of
box.info <br>
--
<br>
local tap =
require('tap')
<br>
-local test = tap.test("Tarantool
4688") <br>
-test:plan(1)
<br>
~
<br>
box.cfg()
<br>
~
<br>
-a =
box.info.memory()
<br>
-b =
box.info:memory()
<br>
+local test =
tap.test('gh-4688-box.info:memory-wrong-result')
<br>
+test:plan(1)
<br>
+
<br>
+local a =
box.info.memory()
<br>
+local b =
box.info:memory()
<br>
+
<br>
+local function
get_keys(t)
<br>
+ local keys =
{} <br>
+ for k, v in pairs(t)
do <br>
+ table.insert(keys,
k) <br>
+
end
<br>
+ return
keys
<br>
+end
<br>
+
<br>
+local keys_1 =
get_keys(box.info.memory())
<br>
+local keys_2 =
get_keys(box.info:memory())
<br>
+test:is_deeply(keys_1, keys_2, "box.info:memory coincide with
box.info.memory") <br>
~
<br>
-test:is(table.concat(a), table.concat(b),
"box.info:memory") <br>
-os.exit(0)
<br>
+os.exit(test:check() and 0 or 1) <br>
<blockquote type="cite"
cite="mid:20200709010804.7kejojfasvu7o7oj@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">
Feel free to use any variant or provide your own.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+os.exit(0)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please, set exit code appropriately (see [1]).
[1]: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/doc/issues/1004">https://github.com/tarantool/doc/issues/1004</a>
</pre>
</blockquote>
</body>
</html>