> +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.
> +$(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?
> +-- | 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.
> 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.
> +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?
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?
> 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.
--
Klaus Aehlig
Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores
