From 235f7d5a95687eac837dfd6248405540269ed581 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Thu, 20 Jan 2022 22:38:51 +0100 Subject: [PATCH] kvserver: use EncodedError in SnapshotResponse We were previously using a "Message" string to indicate details about an error. We can do so much better now and actually encode the error. This wasn't possible when this field was first added, but it is now, so let's use it. As always, there's a migration concern, which means the old field stays around & is populated as well as interpreted for one release. I picked this up for #74621, for which I intend to send a follow-up. Release note: None --- pkg/kv/kvserver/raft.proto | 11 +++++++++++ pkg/kv/kvserver/raft_transport.go | 7 +++++-- pkg/kv/kvserver/store_raft.go | 5 +++-- pkg/kv/kvserver/store_snapshot.go | 14 +++++++++----- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/kv/kvserver/raft.proto b/pkg/kv/kvserver/raft.proto index a11ac8fda205..2a63c5a48893 100644 --- a/pkg/kv/kvserver/raft.proto +++ b/pkg/kv/kvserver/raft.proto @@ -12,6 +12,7 @@ syntax = "proto3"; package cockroach.kv.kvserver; option go_package = "kvserver"; +import "errorspb/errors.proto"; import "roachpb/errors.proto"; import "roachpb/metadata.proto"; import "kv/kvserver/liveness/livenesspb/liveness.proto"; @@ -210,8 +211,18 @@ message SnapshotResponse { reserved 4; } Status status = 1; + // Message is a message explaining an ERROR return value. It is not set for any + // other status. + // + // DEPRECATED: this field can be removed in 22.2. As of 22.1, the encoded_error + // field is always used instead. (22.1 itself needs to populate both due to + // needing to be wire-compatible with 21.2). string message = 2; reserved 3; + // encoded_error encodes the error when the status is ERROR. + // + // MIGRATION: only guaranteed to be set when the message field is no longer there. + errorspb.EncodedError encoded_error = 4 [(gogoproto.nullable) = false]; } // ConfChangeContext is encoded in the raftpb.ConfChange.Context field. diff --git a/pkg/kv/kvserver/raft_transport.go b/pkg/kv/kvserver/raft_transport.go index 44a6ebe6f41a..7275d612308c 100644 --- a/pkg/kv/kvserver/raft_transport.go +++ b/pkg/kv/kvserver/raft_transport.go @@ -420,9 +420,12 @@ func (t *RaftTransport) RaftSnapshot(stream MultiRaft_RaftSnapshotServer) error return err } if req.Header == nil { + err := errors.New("client error: no header in first snapshot request message") return stream.Send(&SnapshotResponse{ - Status: SnapshotResponse_ERROR, - Message: "client error: no header in first snapshot request message"}) + Status: SnapshotResponse_ERROR, + Message: err.Error(), + EncodedError: errors.EncodeError(ctx, err), + }) } rmr := req.Header.RaftMessageRequest handler, ok := t.getHandler(rmr.ToReplica.StoreID) diff --git a/pkg/kv/kvserver/store_raft.go b/pkg/kv/kvserver/store_raft.go index e7cc24dba5f1..521255020e61 100644 --- a/pkg/kv/kvserver/store_raft.go +++ b/pkg/kv/kvserver/store_raft.go @@ -74,8 +74,9 @@ func (s *Store) HandleSnapshot( if s.IsDraining() { return stream.Send(&SnapshotResponse{ - Status: SnapshotResponse_ERROR, - Message: storeDrainingMsg, + Status: SnapshotResponse_ERROR, + Message: storeDrainingMsg, + EncodedError: errors.EncodeError(ctx, errors.Errorf("%s", storeDrainingMsg)), }) } diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go index e5ed6dd91677..ba9963c71655 100644 --- a/pkg/kv/kvserver/store_snapshot.go +++ b/pkg/kv/kvserver/store_snapshot.go @@ -695,8 +695,9 @@ func (s *Store) receiveSnapshot( func sendSnapshotError(stream incomingSnapshotStream, err error) error { return stream.Send(&SnapshotResponse{ - Status: SnapshotResponse_ERROR, - Message: err.Error(), + Status: SnapshotResponse_ERROR, + Message: err.Error(), + EncodedError: errors.EncodeError(context.Background(), err), }) } @@ -1041,9 +1042,12 @@ func sendSnapshot( } switch resp.Status { case SnapshotResponse_ERROR: - storePool.throttle(throttleFailed, resp.Message, to.StoreID) - return errors.Errorf("%s: remote couldn't accept %s with error: %s", - to, snap, resp.Message) + err := errors.Errorf("%s", resp.Message) + if resp.EncodedError.Error != nil { + err = errors.DecodeError(ctx, resp.EncodedError) + } + storePool.throttle(throttleFailed, err.Error(), to.StoreID) + return errors.Wrapf(err, "%s: remote couldn't accept %s", to, snap) case SnapshotResponse_ACCEPTED: // This is the response we're expecting. Continue with snapshot sending. default: