robert feedback

Author: Dieterbe

api/graphite.go

@@ -226,7 +226,11 @@ func (s *Server) renderMetrics(ctx *middleware.Context, request models.GraphiteR
 		mdp = 0
 	}
 
-	opts := optimizations.ApplyUserPrefs(request.Optimizations)
+	opts, err := optimizations.ApplyUserPrefs(request.Optimizations)
+	if err != nil {
+		response.Write(ctx, response.NewError(http.StatusBadRequest, err.Error()))
+		return
+	}
 	plan, err := expr.NewPlan(exprs, fromUnix, toUnix, mdp, stable, opts)
 	if err != nil {
 		if fun, ok := err.(expr.ErrUnknownFunction); ok {
@@ -1422,7 +1426,11 @@ func (s *Server) showPlan(ctx *middleware.Context, request models.GraphiteRender
 	stable := request.Process == "stable"
 	mdp := request.MaxDataPoints
 
-	opts := optimizations.ApplyUserPrefs(request.Optimizations)
+	opts, err := optimizations.ApplyUserPrefs(request.Optimizations)
+	if err != nil {
+		response.Write(ctx, response.NewError(http.StatusBadRequest, err.Error()))
+		return
+	}
 	plan, err := expr.NewPlan(exprs, fromUnix, toUnix, mdp, stable, opts)
 	if err != nil {
 		response.Write(ctx, response.NewError(http.StatusBadRequest, err.Error()))

api/graphite_req.go

@@ -21,6 +21,7 @@ func NewReqMap() *ReqMap {
 	}
 }
 
+// Add adds a models.Req to the ReqMap
 func (r *ReqMap) Add(req models.Req) {
 	r.cnt++
 	if req.PNGroup == 0 {
@@ -29,6 +30,8 @@ func (r *ReqMap) Add(req models.Req) {
 	}
 	r.pngroups[req.PNGroup] = append(r.pngroups[req.PNGroup], req)
 }
+
+// Dump provides a human readable string representation of the ReqsMap
 func (r ReqMap) Dump() string {
 	out := fmt.Sprintf("ReqsMap (%d entries):\n", r.cnt)
 	out += "  Groups:\n"
@@ -51,13 +54,14 @@ type PNGroupSplit struct {
 	mdpno  []models.Req // not MDP-optimizable reqs
 }
 
-// ReqsPlan holds requests that have been planned
+// ReqsPlan holds requests that have been planned, broken down by PNGroup and MDP-optimizability
 type ReqsPlan struct {
 	pngroups map[models.PNGroup]PNGroupSplit
 	single   PNGroupSplit
 	cnt      uint32
 }
 
+// NewReqsPlan generates a ReqsPlan based on the provided ReqMap.
 func NewReqsPlan(reqs ReqMap) ReqsPlan {
 	rp := ReqsPlan{
 		pngroups: make(map[models.PNGroup]PNGroupSplit),
@@ -84,6 +88,7 @@ func NewReqsPlan(reqs ReqMap) ReqsPlan {
 	return rp
 }
 
+// PointsFetch returns how many points this plan will fetch when executed
 func (rp ReqsPlan) PointsFetch() uint32 {
 	var cnt uint32
 	for _, r := range rp.single.mdpyes {
@@ -103,6 +108,7 @@ func (rp ReqsPlan) PointsFetch() uint32 {
 	return cnt
 }
 
+// Dump provides a human readable string representation of the ReqsPlan
 func (rp ReqsPlan) Dump() string {
 	out := fmt.Sprintf("ReqsPlan (%d entries):\n", rp.cnt)
 	out += "  Groups:\n"
@@ -148,6 +154,7 @@ func (rp ReqsPlan) PointsReturn(planMDP uint32) uint32 {
 	return cnt
 }
 
+// List returns the requests contained within the plan as a slice
 func (rp ReqsPlan) List() []models.Req {
 	l := make([]models.Req, 0, rp.cnt)
 	l = append(l, rp.single.mdpno...)

api/models/request.go

@@ -52,7 +52,7 @@ type Req struct {
 // PNGroup is an identifier for a pre-normalization group: data that can be pre-normalized together
 type PNGroup uint64
 
-// NewReq creates a new request. It sets all properties minus the ones that need request planning
+// NewReq creates a new request. It sets all properties except the ones that need request planning
 func NewReq(key schema.MKey, target, patt string, from, to, maxPoints, rawInterval uint32, pngroup PNGroup, cons, consReq consolidation.Consolidator, node cluster.Node, schemaId, aggId uint16) Req {
 	return Req{
 		MKey:         key,
@@ -72,7 +72,7 @@ func NewReq(key schema.MKey, target, patt string, from, to, maxPoints, rawInterv
 }
 
 // Init initializes a request based on the metadata that we know of.
-// It sets all properties minus the ones that need request planning
+// It sets all properties except the ones that need request planning
 func (r *Req) Init(archive idx.Archive, cons consolidation.Consolidator, node cluster.Node) {
 	r.MKey = archive.Id
 	r.Target = archive.NameWithTags()
@@ -97,6 +97,7 @@ func (r *Req) Plan(i int, ret conf.Retention) {
 	r.AggNum = 1
 }
 
+// PlanNormalization updates the planning parameters to accommodate normalization to the specified interval
 func (r *Req) PlanNormalization(interval uint32) {
 	r.OutInterval = interval
 	r.AggNum = interval / r.ArchInterval
@@ -130,6 +131,7 @@ func (r *Req) AdjustTo(interval, from uint32, rets []conf.Retention) {
 	r.PlanNormalization(interval)
 }
 
+// PointsFetch returns how many points this request will fetch when executed
 func (r Req) PointsFetch() uint32 {
 	return (r.To - r.From) / r.ArchInterval
 }

devdocs/render-request-handling.md

@@ -21,16 +21,16 @@
 
 ## MDP-optimization
 
-MDP at the leaf of the expr tree (fetch request) 0 means don't optimize, set it to >0 means, can be optimized.
+MDP at the leaf of the expr tree (fetch request) of 0 means don't optimize.  If set it to >0 it means the request can be optimized.
 When the data may be subjected to a GR-function, we set it to 0.
 How do we achieve this?
-* MDP at the root is set 0 if request came from graphite or to MaxDataPoints otherwise.
-* as the context flows from root through the processing functions to the data requests, if we hit a GR function, we set to MDP to 0 on the context (and thus also on any subsequent requests)
+* MDP at the root is set 0 if the request came from graphite or to MaxDataPoints otherwise.
+* as the context flows from root through the processing functions to the data requests, if we hit a GR function, we set MDP to 0 on the context (and thus also on any subsequent requests)
 
 ## Pre-normalization
 
 Any data requested (checked at the leaf node of the expr tree) should have its own independent interval.
-However, multiple series getting fetched that then get aggregated together, may be pre-normalized if they are part of the same pre-normalization-group. ( have a common PNGroup that is > 0 )
+However, multiple series getting fetched that then get aggregated together may be pre-normalized if they are part of the same pre-normalization-group (have a common PNGroup that is > 0).
 (for more details see devdocs/alignrequests-too-course-grained.txt)
 The mechanics here are:
 * we set PNGroup to 0 by default on the context, which gets inherited down the tree

expr/func_divideseries.go

@@ -61,7 +61,7 @@ func (s *FuncDivideSeries) Exec(cache map[Req][]models.Series) ([]models.Series,
 	// and we'll possibly need to normalize it to different intervals (if the dividends have differing intervals)
 	// (we also need to normalize if there's only 1 dividend but it has a different interval than the divisor)
 	// so let's keep track of the different "versions" of the divisor that we have available.
-	// (the dividend(s) may also need to be normalized but we only use them once so the require no special attention)
+	// (the dividend(s) may also need to be normalized but we only use them once so they don't require special attention)
 	divisorsByRes := make(map[uint32]models.Series)
 	divisorsByRes[divisors[0].Interval] = divisors[0]
 	for _, dividend := range dividends {

expr/plan.go

@@ -15,27 +15,29 @@ type Optimizations struct {
 	MDP              bool
 }
 
-func (o Optimizations) ApplyUserPrefs(s string) Optimizations {
+func (o Optimizations) ApplyUserPrefs(s string) (Optimizations, error) {
 	// no user override. stick to what we have
 	if s == "" {
-		return o
+		return o, nil
 	}
 	// user passed an override. it's either 'none' (no optimizations) or a list of the ones that should be enabled
 	o.PreNormalization = false
 	o.MDP = false
 	if s == "none" {
-		return o
+		return o, nil
 	}
 	prefs := strings.Split(s, ",")
 	for _, pref := range prefs {
-		if pref == "pn" {
+		switch pref {
+		case "pn":
 			o.PreNormalization = true
-		}
-		if pref == "mdp" {
+		case "mdp":
 			o.MDP = true
+		default:
+			return o, fmt.Errorf("unrecognized optimization %q", pref)
 		}
 	}
-	return o
+	return o, nil
 }
 
 // Req represents a request for one/more series