From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E519742F4AD for ; Thu, 18 Jun 2020 18:43:08 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id c21so3757242lfb.3 for ; Thu, 18 Jun 2020 08:43:08 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: =?utf-8?B?0JjQu9GM0Y8g0JrQvtC90Y7RhdC+0LI=?= In-Reply-To: Date: Thu, 18 Jun 2020 18:42:58 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8DEA1673-33E4-4930-9C61-45DEC371FE73@gmail.com> References: <67c75c01-8503-2355-e1f7-9644def2179c@tarantool.org> <6b8b0f63-0a3f-d3ae-7596-2611ea433d44@tarantool.org> <6D02837E-BBEB-4CB9-806F-D5D8E66014B1@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 2/2] feedback: collect db engines and index features List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi, I=E2=80=99ve refactored code a bit according to your comment. See below. > On 18 Jun 2020, at 01:53, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the fixes! >=20 >> I=E2=80=99ve worked on your comments and fixed lapses. See comments = below. >>=20 >> Besides that, are you ok with pushing this (Lua) patch forward? >=20 > I am ok. >=20 >>>> +local function fill_in_features_impl(features) >>>> + fill_in_space_stats(features) >>>> +end >>>> + >>>> +local cached_schema_version =3D 0 >>>> +local cached_feedback_features =3D {} >>>=20 >>> 4. I would better move the cache handling into = fill_in_space_stats(). >>> Because when you will add more features, not related to the schema, >>> they won't relate to schema version. >>>=20 >>> fill_in_features() should not use any caches. Does not depend on = schema. >>> fill_in_space_stats() can use the cache. Because fetches info from = the >>> schema. >>>=20 >>> I mean, it works now, but we would need to rewrite that mostly, when >>> more features will be collected. >>=20 >> Agree. Extracted out caching into fill_in_space_stats(), but kept it = in fill_in_features(), since space_stats is semantically features. >=20 > This is a little bit better but basically is still the same. Just add > a few dummy functions like >=20 > fill_in_swim() > fill_in_box_update() > fill_in_vinyl_stat() > fill_in_netbox() >=20 > which would need to add several new fields to the feedback.features = table, > and see how hard it is to fit it into this code without rewriting it > completely. Ideally addition of such new functions should not change a > single line of the existing code. >=20 > Perhaps you may need to store schema features in = feedback.features.schema, > not in feedback.features. So the schema becomes isolated from the = other > features. And store the others like feedback.features.swim, > feedback.features.box_update, feedback.features.vinyl, etc. Not in a = huge > flat table. I=E2=80=99ve thought about it in the first run, but decided to stick to = the flat approach. Now within a context of us adding more features into = a report it seems more reasonable to me. Fixed that and put schema = statistics into feedback.features.schema field. diff --git a/src/box/lua/feedback_daemon.lua = b/src/box/lua/feedback_daemon.lua index 111a70fca..ee58575a4 100644 --- a/src/box/lua/feedback_daemon.lua +++ b/src/box/lua/feedback_daemon.lua @@ -106,7 +106,7 @@ local function fill_in_indices_stats(space, stats) end end =20 -local function fill_in_space_stats_impl(features) +local function fill_in_schema_stats_impl(schema) local spaces =3D { memtx =3D 0, vinyl =3D 0, @@ -143,30 +143,31 @@ local function fill_in_space_stats_impl(features) end =20 for k, v in pairs(spaces) do - features[k..'_spaces'] =3D v + schema[k..'_spaces'] =3D v end =20 for k, v in pairs(indices) do - features[k..'_indices'] =3D v + schema[k..'_indices'] =3D v end end =20 local cached_schema_version =3D 0 -local cached_feedback_features =3D {} +local cached_schema_features =3D {} =20 -local function fill_in_space_stats(feedback) +local function fill_in_schema_stats(features) local schema_version =3D box.internal.schema_version() if cached_schema_version < schema_version then - local features =3D {} - fill_in_space_stats_impl(features) + local schema =3D {} + fill_in_schema_stats_impl(schema) cached_schema_version =3D schema_version - cached_feedback_features =3D features + cached_schema_features =3D schema end - feedback.features =3D cached_feedback_features + features.schema =3D cached_schema_features end =20 local function fill_in_features(feedback) - fill_in_space_stats(feedback) + feedback.features =3D {} + fill_in_schema_stats(feedback.features) end =20 local function fill_in_feedback(feedback)