From ae8c231d84630de74e1681b12048f90bfe185884 Mon Sep 17 00:00:00 2001 From: Austen McClernon Date: Fri, 7 Apr 2023 23:27:42 +0000 Subject: [PATCH] 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: #90141 Release note: None --- .github/CODEOWNERS | 1 + pkg/BUILD.bazel | 2 ++ pkg/kv/kvserver/BUILD.bazel | 2 ++ pkg/kv/kvserver/benignerror/BUILD.bazel | 12 +++++++++ pkg/kv/kvserver/benignerror/benign_error.go | 30 +++++++++++++++++++++ pkg/kv/kvserver/queue.go | 14 ++-------- pkg/kv/kvserver/queue_concurrency_test.go | 3 ++- pkg/kv/kvserver/replica_command.go | 11 ++++---- pkg/kv/kvserver/replicate_queue.go | 8 +++--- 9 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 pkg/kv/kvserver/benignerror/BUILD.bazel create mode 100644 pkg/kv/kvserver/benignerror/benign_error.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 57b6af16134a..9d787911ebe2 100644 --- a/.github/CODEOWNERS +++ b/.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 diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index f5901620e8d8..a5726dfcbe3d 100644 --- a/pkg/BUILD.bazel +++ b/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", diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index ce1aa608b1f5..cdbbda186336 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/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", diff --git a/pkg/kv/kvserver/benignerror/BUILD.bazel b/pkg/kv/kvserver/benignerror/BUILD.bazel new file mode 100644 index 000000000000..2c17eba83c1f --- /dev/null +++ b/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") diff --git a/pkg/kv/kvserver/benignerror/benign_error.go b/pkg/kv/kvserver/benignerror/benign_error.go new file mode 100644 index 000000000000..789f4bb5efca --- /dev/null +++ b/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)) +} diff --git a/pkg/kv/kvserver/queue.go b/pkg/kv/kvserver/queue.go index 79aa86a46011..251693947350 100644 --- a/pkg/kv/kvserver/queue.go +++ b/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. // diff --git a/pkg/kv/kvserver/queue_concurrency_test.go b/pkg/kv/kvserver/queue_concurrency_test.go index a5d5ffe3f424..30231c301a70 100644 --- a/pkg/kv/kvserver/queue_concurrency_test.go +++ b/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{} }, diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 12f7082c0650..a74fc5b6269f 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/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 } diff --git a/pkg/kv/kvserver/replicate_queue.go b/pkg/kv/kvserver/replicate_queue.go index eee2d7c37c36..06ee300ea1f9 100644 --- a/pkg/kv/kvserver/replicate_queue.go +++ b/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(