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

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

The code and the test look okay.

The explanation of changes may be misleading. There are no formal rules
here, but I made attempt to comment it from correctness point of view.

WBR, Alexander Turenko.

On Thu, Jul 09, 2020 at 04:57:50PM +0300, Olga Arkhangelskaia wrote:
> 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>

__serilize, ymal. Unicode quotes. Over 72 symbol lines.

The link to the lua manual is written twice: in-place and in markdown
reference link style.

Please, ensure that your client sends correct text/plain layer. See the
cited text above and the entry in tarantool-patches archive [1].

 | An extra argument on Lua stack in the second case is box.info table
 | with __serialize metamethod.

It is correct.

 | When all internal functions are over, memory table is
 | pushed on the stack.

Before the patch box.info() table is changed in the
lbox_info_memory_call() function. Or it is about the state after the
patch? Not clear for me.

 | When console prints the output (converts lua table to yaml format and
 | prints) __serialize metamethod of box.info is called.

It is correct, but it is not about the solved problem. I guess it
appears here due to comment in [2] re __serialize and my suggestions
about clarifying why a behaviour observed in the tarantool console will
look different from a correct description in a commit message ('lua',
'data' and other box.info.memory() fields are not seen in the serialized
box.info:memory() value before the patch). I cited below the suggestions
(in Russian).

 | Alexander Turenko: Без фикса:
 |
 | Индекс -3 указывает на таблицу box.info. В нее докидываются поля
 | (видно в консоли, если убрать метатаблицу с __serialize). Но
 | __serialize эти докинутые поля не показывает, там только конкретные
 | выводятся (см. реализацию).
 |
 | <...>
 |
 | Alexander Turenko: Я бы попробовал описать и посмотрел, как читается
 | для человека вне контекста.
 |
 | Alexander Turenko: Что-то типа «результат в консоли может быть
 | misleading, потому что выводится результат вызова __serialize
 | metamethod, а не raw table».
 |
 | Alexander Turenko: А то действительно в описании будет одно, а если
 | пойти проверять, то можно увидеть другое.

When side notes are mixed into a general problem description, it is hard
to understand the idea.

 | __serilize metamethod of box.info shows only some predefined fields and
 | completely ignores memory table.

Hmm. It is correct, but may be misleading: how (in theory) the
metamethod would consider box.info.memory() table that is created in
some other scope? Or, maybe, you say about 'lua', 'data' and other
box.info.memory() fields (not the 'memory table')? If so, it has more
sense, but anyway: even if they would not be ignored in __serialize, our
expectation is that box.info:memory() will give {lua = <...>, data =
<...>, ...}, not the whole box.info() with 'lua', 'data', ... extra
fields. So it also is not applicable as description of the key problem.

 | To avoid this call, patch pushes an extra empty table on Lua stack.

Not correct. The new table is necessary not due to __serialize
behaviour, but because we want to return {lua = <...>, data = <...>,
...} table, not add those fields into box.info() table and return it.

 | When console calls ymal.serializer to print lua table, box.info will
 | be ignored.

It is correct, but may be misleading. We return another table and so the
serializer will encode it instead of box.info(). So, yep, box.info()
will be ignored, but how it may not be ignored by a console serializer
if we don't return it? And, again, __serialize behaviour is not the key
problem.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018384.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018372.html