Fixed HA Tracker jitter causing unnecessary CAS operations (#1861)

* Fixed HA Tracker jitter causing unnecessary CAS operations

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Added entry to changelog

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Improved HATrackerConfig.Validate()

Signed-off-by: Marco Pracucci <marco@pracucci.com>

* Cleaner checkReplicaTimestamp() test function

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Author: pracucci

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## master / unreleased
 
+* [BUGFIX] Fixed unnecessary CAS operations done by the HA tracker when the jitter is enabled. #1861
+
 ## 0.4.0 / 2019-12-02
 
 * [CHANGE] The frontend component has been refactored to be easier to re-use. When upgrading the frontend, cache entries will be discarded and re-created with the new protobuf schema. #1734

pkg/distributor/ha_tracker.go

@@ -2,6 +2,7 @@ package distributor
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"math/rand"
@@ -48,6 +49,9 @@ var (
 		Name:      "ha_tracker_kv_store_cas_total",
 		Help:      "The total number of CAS calls to the KV store for a user ID/cluster.",
 	}, []string{"user", "cluster"})
+
+	errNegativeUpdateTimeoutJitterMax = errors.New("HA tracker max update timeout jitter shouldn't be negative")
+	errInvalidFailoverTimeout         = "HA Tracker failover timeout (%v) must be at least 1s greater than update timeout - max jitter (%v)"
 )
 
 // ProtoReplicaDescFactory makes new InstanceDescs
@@ -117,10 +121,15 @@ func (cfg *HATrackerConfig) RegisterFlags(f *flag.FlagSet) {
 
 // Validate config and returns error on failure
 func (cfg *HATrackerConfig) Validate() error {
-	if cfg.FailoverTimeout < cfg.UpdateTimeout+cfg.UpdateTimeoutJitterMax+time.Second {
-		return fmt.Errorf("HA Tracker failover timeout (%v) must be at least 1s greater than update timeout (%v)",
-			cfg.FailoverTimeout, cfg.UpdateTimeout+cfg.UpdateTimeoutJitterMax+time.Second)
+	if cfg.UpdateTimeoutJitterMax < 0 {
+		return errNegativeUpdateTimeoutJitterMax
+	}
+
+	minFailureTimeout := cfg.UpdateTimeout + cfg.UpdateTimeoutJitterMax + time.Second
+	if cfg.FailoverTimeout < minFailureTimeout {
+		return fmt.Errorf(errInvalidFailoverTimeout, cfg.FailoverTimeout, minFailureTimeout)
 	}
+
 	return nil
 }
 
@@ -207,7 +216,7 @@ func (c *haTracker) checkReplica(ctx context.Context, userID, cluster, replica s
 	c.electedLock.RLock()
 	entry, ok := c.elected[key]
 	c.electedLock.RUnlock()
-	if ok && now.Sub(timestamp.Time(entry.ReceivedAt)) < c.cfg.UpdateTimeout {
+	if ok && now.Sub(timestamp.Time(entry.ReceivedAt)) < c.cfg.UpdateTimeout+c.updateTimeoutJitter {
 		if entry.Replica != replica {
 			return replicasNotMatchError(replica, entry.Replica)
 		}