kvserver; move benignerror into pkg

Benign error is used to declare an error that is not u enough to log an
error when returned in queue processing. As part of a refactor to remove
decision logic out of the replicate queue, this patch moves benign error
into its own, separate package.

Part of: cockroachdb#90141

Release note: None

.github/CODEOWNERS

@@ -233,6 +233,7 @@
 /pkg/kv/kvserver/apply/                 @cockroachdb/repl-prs
 /pkg/kv/kvserver/asim/                  @cockroachdb/kv-prs
 /pkg/kv/kvserver/batcheval/             @cockroachdb/kv-prs
+/pkg/kv/kvserver/benignerror/           @cockroachdb/kv-prs
 /pkg/kv/kvserver/closedts/              @cockroachdb/repl-prs
 /pkg/kv/kvserver/concurrency/           @cockroachdb/kv-prs
 /pkg/kv/kvserver/constraint/            @cockroachdb/kv-prs

pkg/BUILD.bazel

@@ -1259,6 +1259,7 @@ GO_TARGETS = [
     "//pkg/kv/kvserver/batcheval/result:result_test",
     "//pkg/kv/kvserver/batcheval:batcheval",
     "//pkg/kv/kvserver/batcheval:batcheval_test",
+    "//pkg/kv/kvserver/benignerror:benignerror",
     "//pkg/kv/kvserver/closedts/ctpb:ctpb",
     "//pkg/kv/kvserver/closedts/sidetransport:sidetransport",
     "//pkg/kv/kvserver/closedts/sidetransport:sidetransport_test",
@@ -2721,6 +2722,7 @@ GET_X_DATA_TARGETS = [
     "//pkg/kv/kvserver/asim/workload:get_x_data",
     "//pkg/kv/kvserver/batcheval:get_x_data",
     "//pkg/kv/kvserver/batcheval/result:get_x_data",
+    "//pkg/kv/kvserver/benignerror:get_x_data",
     "//pkg/kv/kvserver/closedts:get_x_data",
     "//pkg/kv/kvserver/closedts/ctpb:get_x_data",
     "//pkg/kv/kvserver/closedts/sidetransport:get_x_data",

pkg/kv/kvserver/BUILD.bazel

@@ -124,6 +124,7 @@ go_library(
         "//pkg/kv/kvserver/apply",
         "//pkg/kv/kvserver/batcheval",
         "//pkg/kv/kvserver/batcheval/result",
+        "//pkg/kv/kvserver/benignerror",
         "//pkg/kv/kvserver/closedts",
         "//pkg/kv/kvserver/closedts/ctpb",
         "//pkg/kv/kvserver/closedts/sidetransport",
@@ -363,6 +364,7 @@ go_test(
         "//pkg/kv/kvserver/apply",
         "//pkg/kv/kvserver/batcheval",
         "//pkg/kv/kvserver/batcheval/result",
+        "//pkg/kv/kvserver/benignerror",
         "//pkg/kv/kvserver/closedts",
         "//pkg/kv/kvserver/closedts/ctpb",
         "//pkg/kv/kvserver/closedts/tracker",

pkg/kv/kvserver/benignerror/BUILD.bazel

@@ -0,0 +1,12 @@
+load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "benignerror",
+    srcs = ["benign_error.go"],
+    importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/benignerror",
+    visibility = ["//visibility:public"],
+    deps = ["@com_github_cockroachdb_errors//:errors"],
+)
+
+get_x_data(name = "get_x_data")

pkg/kv/kvserver/benignerror/benign_error.go

@@ -0,0 +1,30 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+package benignerror
+
+import "github.com/cockroachdb/errors"
+
+// BenignError may be used for declaring an error that is less serious i.e.
+// benign.
+type BenignError struct {
+	cause error
+}
+
+// New returns a new benign error with the given error cause.
+func New(cause error) *BenignError {
+	return &BenignError{cause: cause}
+}
+
+func (be *BenignError) Error() string { return be.cause.Error() }
+func (be *BenignError) Cause() error  { return be.cause }
+
+func IsBenign(err error) bool {
+	return errors.HasType(err, (*BenignError)(nil))
+}

pkg/kv/kvserver/queue.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/benignerror"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -1021,17 +1022,6 @@ func (bq *baseQueue) processReplica(ctx context.Context, repl replicaInQueue) er
 		})
 }
 
-type benignError struct {
-	cause error
-}
-
-func (be *benignError) Error() string { return be.cause.Error() }
-func (be *benignError) Cause() error  { return be.cause }
-
-func isBenign(err error) bool {
-	return errors.HasType(err, (*benignError)(nil))
-}
-
 // IsPurgatoryError returns true iff the given error is a purgatory error.
 func IsPurgatoryError(err error) (PurgatoryError, bool) {
 	var purgErr PurgatoryError
@@ -1118,7 +1108,7 @@ func (bq *baseQueue) finishProcessingReplica(
 
 	// Handle failures.
 	if err != nil {
-		benign := isBenign(err)
+		benign := benignerror.IsBenign(err)
 
 		// Increment failures metric.
 		//

pkg/kv/kvserver/queue_concurrency_test.go

@@ -18,6 +18,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/benignerror"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -86,7 +87,7 @@ func TestBaseQueueConcurrent(t *testing.T) {
 			} else if n == 1 {
 				return false, errors.New("injected regular error")
 			} else if n == 2 {
-				return false, &benignError{errors.New("injected benign error")}
+				return false, benignerror.New(errors.New("injected benign error"))
 			}
 			return false, &testPurgatoryError{}
 		},

pkg/kv/kvserver/replica_command.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/allocatorimpl"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/benignerror"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -387,7 +388,7 @@ func (r *Replica) adminSplitWithDescriptor(
 			if ok, actualDesc := maybeDescriptorChangedError(desc, err); ok {
 				// NB: we have to wrap the existing error here as consumers of this code
 				// look at the root cause to sniff out the changed descriptor.
-				err = &benignError{wrapDescChangedError(err, desc, actualDesc)}
+				err = benignerror.New(wrapDescChangedError(err, desc, actualDesc))
 			}
 			return reply, err
 		}
@@ -421,7 +422,7 @@ func (r *Replica) adminSplitWithDescriptor(
 		if ok, actualDesc := maybeDescriptorChangedError(desc, err); ok {
 			// NB: we have to wrap the existing error here as consumers of this code
 			// look at the root cause to sniff out the changed descriptor.
-			err = &benignError{wrapDescChangedError(err, desc, actualDesc)}
+			err = benignerror.New(wrapDescChangedError(err, desc, actualDesc))
 		}
 		return reply, errors.Wrapf(err, "split at key %s failed", splitKey)
 	}
@@ -497,7 +498,7 @@ func (r *Replica) adminUnsplitWithDescriptor(
 		if ok, actualDesc := maybeDescriptorChangedError(desc, err); ok {
 			// NB: we have to wrap the existing error here as consumers of this code
 			// look at the root cause to sniff out the changed descriptor.
-			err = &benignError{wrapDescChangedError(err, desc, actualDesc)}
+			err = benignerror.New(wrapDescChangedError(err, desc, actualDesc))
 		}
 		return reply, err
 	}
@@ -2437,7 +2438,7 @@ func execChangeReplicasTxn(
 			// as "secondary payload", in case the error object makes it way
 			// to logs or telemetry during a crash.
 			err = errors.WithSecondaryError(newDescChangedError(referenceDesc, actualDesc), err)
-			err = &benignError{err}
+			err = benignerror.New(err)
 		}
 		return nil, errors.Wrapf(err, "change replicas of r%d failed", referenceDesc.RangeID)
 	}
@@ -2780,7 +2781,7 @@ func (r *Replica) sendSnapshotUsingDelegate(
 	if status == nil {
 		// This code path is sometimes hit during scatter for replicas that
 		// haven't woken up yet.
-		retErr = &benignError{errors.Wrap(errMarkSnapshotError, "raft status not initialized")}
+		retErr = benignerror.New(errors.Wrap(errMarkSnapshotError, "raft status not initialized"))
 		return
 	}
 

pkg/kv/kvserver/replicate_queue.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/allocatorimpl"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/allocator/storepool"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/benignerror"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb"
@@ -1385,7 +1386,7 @@ func (rq *replicateQueue) findRemoveVoter(
 		raftStatus := repl.RaftStatus()
 		if raftStatus == nil || raftStatus.RaftState != raft.StateLeader {
 			// If we've lost raft leadership, we're unlikely to regain it so give up immediately.
-			return roachpb.ReplicationTarget{}, "", &benignError{errors.Errorf("not raft leader while range needs removal")}
+			return roachpb.ReplicationTarget{}, "", benignerror.New(errors.Errorf("not raft leader while range needs removal"))
 		}
 		candidates = allocatorimpl.FilterUnremovableReplicas(ctx, raftStatus, existingVoters, lastReplAdded)
 		log.KvDistribution.VEventf(ctx, 3, "filtered unremovable replicas from %v to get %v as candidates for removal: %s",
@@ -1416,12 +1417,11 @@ func (rq *replicateQueue) findRemoveVoter(
 	}
 	if len(candidates) == 0 {
 		// If we timed out and still don't have any valid candidates, give up.
-		return roachpb.ReplicationTarget{}, "", &benignError{
+		return roachpb.ReplicationTarget{}, "", benignerror.New(
 			errors.Errorf(
 				"no removable replicas from range that needs a removal: %s",
 				rangeRaftProgress(repl.RaftStatus(), existingVoters),
-			),
-		}
+			))
 	}
 
 	return rq.allocator.RemoveVoter(