From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 E469B445320 for ; Thu, 9 Jul 2020 04:08:50 +0300 (MSK) Date: Thu, 9 Jul 2020 04:08:04 +0300 From: Alexander Turenko Message-ID: <20200709010804.7kejojfasvu7o7oj@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> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <16d471e3-d578-7b12-6a4f-08814a2f1b64@tarantool.org> 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 Pasted the actual patch to comment in-place. > commit ad00de576ab0300a3e48be2cdda2bef5938eb40e > Author: Olga Arkhangelskaia > 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 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 '' or nil | b.lua = b.lua and '' 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