From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 88D7A445320 for ; Thu, 16 Jul 2020 21:06:36 +0300 (MSK) Date: Thu, 16 Jul 2020 20:56:18 +0300 From: Igor Munkin Message-ID: <20200716175618.GN5559@tarantool.org> 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> <20200715144023.pll42732ijjw6obo@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200715144023.pll42732ijjw6obo@tkn_work_nb> 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, Alexander Turenko Olya, Thanks for your updates! The patch is OK, but I still have several comments regarding it, so I pasted it below. Besides, the branch is extremely outdated and based on 2.2 (it might complicate the merge process). | box: fix box.info:memory() | | Fix the return value of box.info:memory(). It used to return the same table | as the box.info(). | | If one removes metatable of the box.info:memory(): | tarantool> setmetatable(box.info:memory(), nil) | --- | - cache: 0 | lua: 1855568 | data: 37976 | index: 1196032 | net: 589824 | tx: 0 | version: 2.5.0-158-g667145aff | package: Tarantool | | While printing the table, lbox_info_memory_call adds box.info.memory's fields to | the box.info table. To avoid this effect and return just box.info.memory we put | an empty table on the Lua stack at the beginning of the lbox_info_memory_call | function. | | Closes #4688 At first, the message still exceeds 72 symbols. Sasha has already pointed to this several times. Let's make Sasha a bit happier! Furthermore, you described why the output is the same as the one obtained via box.info, but stripped everything regarding why box.info is used here. I still guess it's worth to be mentioned, even if it is perfectly described in Lua reference manual and PIL. | diff --git a/src/box/lua/info.c b/src/box/lua/info.c | index d0e553b1d..e5b1c32f0 100644 | --- a/src/box/lua/info.c | +++ b/src/box/lua/info.c | @@ -322,6 +322,8 @@ lbox_info_memory_call(struct lua_State *L) | struct engine_memory_stat stat; | engine_memory_stat(&stat); | | + lua_newtable(L); | + I agree with Sasha, regarding usage: it can preallocate the exact required size memory area, so I see no reasons to ignore such benefit. | 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..8d6aa0e00 | --- /dev/null | +++ b/test/box-tap/gh-4688-box-info-memory.test.lua | @@ -0,0 +1,28 @@ | +#!/usr/bin/env tarantool | + | +-- | +-- gh-4688: box.info:memory() displayed full content of box.info | +-- | +local tap = require('tap') | + | +box.cfg() | + | +local test = tap.test('gh-4688-box.info:memory-wrong-result') | +test:plan(1) | + | +local a = box.info.memory() | +local b = box.info:memory() These variables are excess. | + | +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") | + | +os.exit(test:check() and 0 or 1) -- Best regards, IM