Dear Klaus,
thanks for the review.
+import Ganeti.HTools.Cluster.MetricsTH
+
+-- data NodeValues { ... }
+$(nodeValuesDecl M.metricComponents)
Normally, we have all explanations about what a template
haskell function does at the place where the function is declared.
Since the great picture now ends up in the MetricsTH file, you
might as well combine things there, e.g.,
declareStatistics :: [MetricComponent] -> Q [Dec]
declareStatistics components = do
nodesValues <- nodeValuesDecl components
clusterStatistics <- clusterStatisticsDecl
...
return $ nodeValues ++ clusterStatistics ++ ...
and in this file just have a simple
$(declareStatistics M.MetricComponents)
+-- getNodeValues :: Node.Node -> NodeValues
+$(getNodeValuesDecl M.metricComponents)
+-- compClusterStatisticsHelper :: ([Node.Node], [Node.Node])
+-- -> ClusterStatistics
NACK. A function generating a function declaration is supposed to
also generate its type declaration.
.
generates the type declaration. I've add commented type declaration
here only to increase the readability of the code. Do you think that I
should get rid of such comments?
+$(compClusterStatisticsHelperDecl M.metricComponents)
+-- updateClusterStatistics :: ClusterStatistics -> (Node.Node, Node.Node)
+-- -> ClusterStatistics
dito
+$(updateClusterStatisticsDecl M.metricComponents)
+-- getStatisticValues :: ClusterStatistics -> [Double]
+$(getStatisticValuesDecl M.metricComponents)
If the whole point is to get rid of these positional list of doubles, why
generate such a function? Can't we just generate directly the functions that
use it, i.e., compCVfromStats and printStats via template haskell?
Yes, it's possible to do it but the code will become a bit more complex.
From my point of view the goal is to implement type safe metrics and
statistics. Moreover, this function is supposed to use only in the
Metrics.hs file. It's possible not to export getStatisticsValues.
Anyway, each statistics finally becomes a double. And cluster statistics
is a linear combination of its components.
+-- | Compute the statistics of a cluster given the nodes
+compClusterStatistics :: [Node.Node] -> ClusterStatistics
+compClusterStatistics nl =
+ compClusterStatisticsHelper $ partition Node.offline nl
Why genrate a helper with template haskell and not directly
compClusterStatistics?
I don't see where the helper is used anywhere else or useful in itself.
Again, to simplify the template haskell code. simple partition
Node.Offline nl requests about 3-4 lines of template haskell code.
diff --git a/src/Ganeti/HTools/Cluster/MetricsComponents.hs
b/src/Ganeti/HTools/Cluster/MetricsComponents.hs
new file mode 100644
index 0000000..64e1095
--- /dev/null
+++ b/src/Ganeti/HTools/Cluster/MetricsComponents.hs
@@ -0,0 +1,308 @@
+{-# LANGUAGE TemplateHaskell #-}
+
+{-| Module describing cluster metrics components.
+
+ Metrics components are used for generation of functions deaing with cluster
+ statistics.
+
+-}
+
+{-
+
+Copyright (C) 2009, 2010, 2011, 2012, 2013 Google Inc.
Since it is a new file, it should say
Copyright (C) 2015 Google Inc.
as no code is taken from other files. This also applies to the
other new file.
Well, of course.
+module Ganeti.HTools.Cluster.MetricsComponents
+ ( MetricComponent
Why not
MetricComponent(..)
instead of exporting all the parts individually? Is there
any part of a metric component you want to hide, and why?
No, no. I'll write it in the proper way.
Also, from an organisational level it might make more sense
to move declaration of the MetricComponent data structure to
the MetricsTH file and here just import the definition; then
you would have one file with all the abstract structure of
the metrics and a file with the concrete components. What do
you think?
Here, I agree with you.
diff --git a/src/Ganeti/Utils/Statistics.hs b/src/Ganeti/Utils/Statistics.hs
[...]
+class (Show c) => Stat s c | c -> s where
Why is Show a prerequisite for this type class? From the description it seems
that it is only about statistical computation. For those there is no need
to show anything; the final double is an instance of Show anyway.
When defining a function that shows something about a statistics, it is
better the require the Show in the signature of that function, instead
of only accepting a thing as a statistics if it can be shown.
And here, I agree again.
--
Sincerely, Oleg Ponomarev <[email protected]>
