Skip to content

Commit

Permalink
kvserver: use EncodedError in SnapshotResponse
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tbg committed Jan 20, 2022
1 parent e8a0b75 commit 235f7d5
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 9 deletions.
11 changes: 11 additions & 0 deletions pkg/kv/kvserver/raft.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 5 additions & 2 deletions pkg/kv/kvserver/raft_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions pkg/kv/kvserver/store_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
})
}

Expand Down
14 changes: 9 additions & 5 deletions pkg/kv/kvserver/store_snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
})
}

Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 235f7d5

Please sign in to comment.