Propagate retriable/haltable errors from compactor

cmd/thanos/compact.go

@@ -285,9 +285,9 @@ func runCompact(
 			if err == nil {
 				return nil
 			}
+
 			// The HaltError type signals that we hit a critical bug and should block
-			// for investigation.
-			// You should alert on this being halted.
+			// for investigation. You should alert on this being halted.
 			if compact.IsHaltError(err) {
 				if haltOnError {
 					level.Error(logger).Log("msg", "critical error detected; halting", "err", err)
@@ -299,7 +299,7 @@ func runCompact(
 			}
 
 			// The RetryError signals that we hit an retriable error (transient error, no connection).
-			// You should alert on this being triggered to frequently.
+			// You should alert on this being triggered too frequently.
 			if compact.IsRetryError(err) {
 				level.Error(logger).Log("msg", "retriable error", "err", err)
 				retried.Inc()

pkg/compact/compact.go

@@ -8,21 +8,20 @@ import (
 	"path"
 	"path/filepath"
 	"sort"
-	"strings"
 	"sync"
 	"time"
 
-	"github.com/improbable-eng/thanos/pkg/block/metadata"
-
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
 	"github.com/improbable-eng/thanos/pkg/block"
+	"github.com/improbable-eng/thanos/pkg/block/metadata"
 	"github.com/improbable-eng/thanos/pkg/compact/downsample"
 	"github.com/improbable-eng/thanos/pkg/objstore"
 	"github.com/oklog/ulid"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/tsdb"
+	terrors "github.com/prometheus/tsdb/errors"
 	"github.com/prometheus/tsdb/labels"
 )
 
@@ -613,7 +612,17 @@ func (e HaltError) Error() string {
 }
 
 // IsHaltError returns true if the base error is a HaltError.
+// If a multierror is passed, any halt error will return true.
 func IsHaltError(err error) bool {
+	if multiErr, ok := err.(terrors.MultiError); ok {
+		for _, err := range multiErr {
+			if _, ok := errors.Cause(err).(HaltError); ok {
+				return true
+			}
+		}
+		return false
+	}
+
 	_, ok := errors.Cause(err).(HaltError)
 	return ok
 }
@@ -636,7 +645,17 @@ func (e RetryError) Error() string {
 }
 
 // IsRetryError returns true if the base error is a RetryError.
+// If a multierror is passed, all errors must be retriable.
 func IsRetryError(err error) bool {
+	if multiErr, ok := err.(terrors.MultiError); ok {
+		for _, err := range multiErr {
+			if _, ok := errors.Cause(err).(RetryError); !ok {
+				return false
+			}
+		}
+		return true
+	}
+
 	_, ok := errors.Cause(err).(RetryError)
 	return ok
 }
@@ -1037,12 +1056,12 @@ func (c *BucketCompactor) Compact(ctx context.Context) error {
 		close(errChan)
 		workCtxCancel()
 		if err != nil {
-			errMsgs := []string{err.Error()}
+			errs := terrors.MultiError{err}
 			// Collect any other errors reported by the workers.
 			for e := range errChan {
-				errMsgs = append(errMsgs, e.Error())
+				errs.Add(e)
 			}
-			return errors.New(strings.Join(errMsgs, "; "))
+			return errs
 		}
 
 		if finishedAllGroups {

pkg/compact/compact_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/improbable-eng/thanos/pkg/testutil"
 	"github.com/oklog/ulid"
 	"github.com/pkg/errors"
+	terrors "github.com/prometheus/tsdb/errors"
 )
 
 func TestHaltError(t *testing.T) {
@@ -27,6 +28,31 @@ func TestHaltError(t *testing.T) {
 	testutil.Assert(t, IsHaltError(err), "not a halt error")
 }
 
+func TestHaltMultiError(t *testing.T) {
+	haltErr := halt(errors.New("halt error"))
+	nonHaltErr := errors.New("not a halt error")
+
+	errs := terrors.MultiError{nonHaltErr}
+	testutil.Assert(t, !IsHaltError(errs), "should not be a halt error")
+
+	errs.Add(haltErr)
+	testutil.Assert(t, IsHaltError(errs), "if any halt errors are present this should return true")
+}
+
+func TestRetryMultiError(t *testing.T) {
+	retryErr := retry(errors.New("retry error"))
+	nonRetryErr := errors.New("not a retry error")
+
+	errs := terrors.MultiError{nonRetryErr}
+	testutil.Assert(t, !IsRetryError(errs), "should not be a retry error")
+
+	errs = terrors.MultiError{retryErr}
+	testutil.Assert(t, IsRetryError(errs), "if all errors are retriable this should return true")
+
+	errs = terrors.MultiError{nonRetryErr, retryErr}
+	testutil.Assert(t, !IsRetryError(errs), "mixed errors should return false")
+}
+
 func TestRetryError(t *testing.T) {
 	err := errors.New("test")
 	testutil.Assert(t, !IsRetryError(err), "retry error")