Re: [Tarantool-patches] [PATCH 2/2] feedback: collect db engines and index features

["Илья Конюхов" <runsfor@gmail.com>]

Hi,

I’ve refactored code a bit according to your comment. See below.

> On 18 Jun 2020, at 01:53, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the fixes!
> 
>> I’ve worked on your comments and fixed lapses. See comments below.
>> 
>> Besides that, are you ok with pushing this (Lua) patch forward?
> 
> I am ok.
> 
>>>> +local function fill_in_features_impl(features)
>>>> +    fill_in_space_stats(features)
>>>> +end
>>>> +
>>>> +local cached_schema_version = 0
>>>> +local cached_feedback_features = {}
>>> 
>>> 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.
>>> 
>>> 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.
>>> 
>>> I mean, it works now, but we would need to rewrite that mostly, when
>>> more features will be collected.
>> 
>> Agree. Extracted out caching into fill_in_space_stats(), but kept it in fill_in_features(), since space_stats is semantically features.
> 
> This is a little bit better but basically is still the same. Just add
> a few dummy functions like
> 
> 	fill_in_swim()
> 	fill_in_box_update()
> 	fill_in_vinyl_stat()
> 	fill_in_netbox()
> 
> 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.
> 
> 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’ve 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
 
-local function fill_in_space_stats_impl(features)
+local function fill_in_schema_stats_impl(schema)
     local spaces = {
         memtx     = 0,
         vinyl     = 0,
@@ -143,30 +143,31 @@ local function fill_in_space_stats_impl(features)
     end
 
     for k, v in pairs(spaces) do
-        features[k..'_spaces'] = v
+        schema[k..'_spaces'] = v
     end
 
     for k, v in pairs(indices) do
-        features[k..'_indices'] = v
+        schema[k..'_indices'] = v
     end
 end
 
 local cached_schema_version = 0
-local cached_feedback_features = {}
+local cached_schema_features = {}
 
-local function fill_in_space_stats(feedback)
+local function fill_in_schema_stats(features)
     local schema_version = box.internal.schema_version()
     if cached_schema_version < schema_version then
-        local features = {}
-        fill_in_space_stats_impl(features)
+        local schema = {}
+        fill_in_schema_stats_impl(schema)
         cached_schema_version = schema_version
-        cached_feedback_features = features
+        cached_schema_features = schema
     end
-    feedback.features = cached_feedback_features
+    features.schema = cached_schema_features
 end
 
 local function fill_in_features(feedback)
-    fill_in_space_stats(feedback)
+    feedback.features = {}
+    fill_in_schema_stats(feedback.features)
 end
 
 local function fill_in_feedback(feedback)