Skip to content

Commit

Permalink
storage: un-embed decodedConfChange
Browse files Browse the repository at this point in the history
I ate a number of NPEs during development because nullable embedded
fields are tricky; they hide the pointer derefs that often need a nil
check. We'll embed the fields of decodedConfChange instead which works
out better.
This commit also adds the unmarshaling code necessary for ConfChangeV2
needed once we issue atomic replication changes.

Release note: None
  • Loading branch information
tbg committed Aug 23, 2019
1 parent efda358 commit c9c39d3
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 13 deletions.
38 changes: 27 additions & 11 deletions pkg/storage/replica_application_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/cockroachdb/cockroach/pkg/util/tracing"
"github.com/cockroachdb/errors"
opentracing "github.com/opentracing/opentracing-go"
"go.etcd.io/etcd/raft/raftpb"
)
Expand Down Expand Up @@ -79,15 +80,15 @@ type replicatedCmd struct {

// decodedRaftEntry represents the deserialized content of a raftpb.Entry.
type decodedRaftEntry struct {
idKey storagebase.CmdIDKey
raftCmd storagepb.RaftCommand
*decodedConfChange // only non-nil for config changes
idKey storagebase.CmdIDKey
raftCmd storagepb.RaftCommand
confChange *decodedConfChange // only non-nil for config changes
}

// decodedConfChange represents the fields of a config change raft command.
type decodedConfChange struct {
cc raftpb.ConfChange
ccCtx ConfChangeContext
raftpb.ConfChangeI
ConfChangeContext
}

// decode decodes the entry e into the replicatedCmd.
Expand Down Expand Up @@ -191,17 +192,32 @@ func (d *decodedRaftEntry) decodeNormalEntry(e *raftpb.Entry) error {
}

func (d *decodedRaftEntry) decodeConfChangeEntry(e *raftpb.Entry) error {
d.decodedConfChange = &decodedConfChange{}
if err := protoutil.Unmarshal(e.Data, &d.cc); err != nil {
return wrapWithNonDeterministicFailure(err, "while unmarshaling ConfChange")
d.confChange = &decodedConfChange{}

switch e.Type {
case raftpb.EntryConfChange:
var cc raftpb.ConfChange
if err := protoutil.Unmarshal(e.Data, &cc); err != nil {
return wrapWithNonDeterministicFailure(err, "while unmarshaling ConfChange")
}
d.confChange.ConfChangeI = cc
case raftpb.EntryConfChangeV2:
var cc raftpb.ConfChangeV2
if err := protoutil.Unmarshal(e.Data, &cc); err != nil {
return wrapWithNonDeterministicFailure(err, "while unmarshaling ConfChangeV2")
}
d.confChange.ConfChangeI = cc
default:
err := errors.New("unknown entry type")
return wrapWithNonDeterministicFailure(err, err.Error())
}
if err := protoutil.Unmarshal(d.cc.Context, &d.ccCtx); err != nil {
if err := protoutil.Unmarshal(d.confChange.AsV2().Context, &d.confChange.ConfChangeContext); err != nil {
return wrapWithNonDeterministicFailure(err, "while unmarshaling ConfChangeContext")
}
if err := protoutil.Unmarshal(d.ccCtx.Payload, &d.raftCmd); err != nil {
if err := protoutil.Unmarshal(d.confChange.Payload, &d.raftCmd); err != nil {
return wrapWithNonDeterministicFailure(err, "while unmarshaling RaftCommand")
}
d.idKey = storagebase.CmdIDKey(d.ccCtx.CommandID)
d.idKey = storagebase.CmdIDKey(d.confChange.CommandID)
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/storage/replica_application_state_machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1001,10 +1001,10 @@ func (sm *replicaStateMachine) maybeApplyConfChange(ctx context.Context, cmd *re
case raftpb.EntryConfChange:
if cmd.replicatedResult().ChangeReplicas == nil {
// The command was rejected.
cmd.cc = raftpb.ConfChange{}
cmd.confChange.ConfChangeI = raftpb.ConfChange{}
}
return sm.r.withRaftGroup(true, func(raftGroup *raft.RawNode) (bool, error) {
raftGroup.ApplyConfChange(cmd.cc)
raftGroup.ApplyConfChange(cmd.confChange.ConfChangeI)
return true, nil
})
default:
Expand Down

0 comments on commit c9c39d3

Please sign in to comment.