From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BA12E445320 for ; Wed, 15 Jul 2020 17:41:09 +0300 (MSK) Date: Wed, 15 Jul 2020 17:40:23 +0300 From: Alexander Turenko Message-ID: <20200715144023.pll42732ijjw6obo@tkn_work_nb> References: <20200629121118.21596-1-arkholga@tarantool.org> <20200629121118.21596-2-arkholga@tarantool.org> <20200701213447.GE5559@tarantool.org> <16d471e3-d578-7b12-6a4f-08814a2f1b64@tarantool.org> <20200709010804.7kejojfasvu7o7oj@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/1] box: fixed box.info:memory() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.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 __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