diff --git a/pkg/kv/kvserver/kvserverbase/forced_error.go b/pkg/kv/kvserver/kvserverbase/forced_error.go index 4a4adbbccb0d..faf6910dc364 100644 --- a/pkg/kv/kvserver/kvserverbase/forced_error.go +++ b/pkg/kv/kvserver/kvserverbase/forced_error.go @@ -95,11 +95,11 @@ func CheckForcedErr( requestedLease = *raftCmd.ReplicatedEvalResult.State.Lease } if idKey == "" { - // This is an empty Raft command (which is sent by Raft after elections - // to trigger reproposals or during concurrent configuration changes). - // Nothing to do here except making sure that the corresponding batch - // (which is bogus) doesn't get executed (for it is empty and so - // properties like key range are undefined). + // This is an empty Raft command, which is sent by Raft after elections to + // trigger reproposals, during concurrent configuration changes, or to + // unquiesce the Raft group. Nothing to do here except making sure that the + // corresponding batch (which is bogus) doesn't get executed (for it is + // empty and so properties like key range are undefined). return ForcedErrResult{ LeaseIndex: leaseIndex, Rejection: ProposalRejectionPermanent, diff --git a/pkg/kv/kvserver/raftlog/entry.go b/pkg/kv/kvserver/raftlog/entry.go index f05d10baa3dd..d68a455cc56e 100644 --- a/pkg/kv/kvserver/raftlog/entry.go +++ b/pkg/kv/kvserver/raftlog/entry.go @@ -183,10 +183,15 @@ func (e *Entry) load() error { } e.ID = kvserverbase.CmdIDKey(e.ConfChangeContext.CommandID) raftCmdBytes = e.ConfChangeContext.Payload + } else if len(raftCmdBytes) == 0 { + // Empty commands may be proposed to wake the leader, e.g. during + // unquiescence. Ignore them during application by clearing the command ID + // and other fields (see CheckForcedErr), similarly to those submitted by + // Raft on leader changes (see EntryEncodingEmpty). + *e = Entry{Entry: e.Entry} + return nil } - // TODO(tbg): can len(payload)==0 if we propose an empty command to wake up leader? - // If so, is that a problem here? return errors.Wrap(protoutil.Unmarshal(raftCmdBytes, &e.Cmd), "unmarshalling RaftCommand") } diff --git a/pkg/kv/kvserver/raftlog/entry_test.go b/pkg/kv/kvserver/raftlog/entry_test.go index 6893d4b3541c..c8987010875f 100644 --- a/pkg/kv/kvserver/raftlog/entry_test.go +++ b/pkg/kv/kvserver/raftlog/entry_test.go @@ -18,17 +18,46 @@ import ( "go.etcd.io/raft/v3/raftpb" ) -func TestLoadInvalidEntry(t *testing.T) { - invalidEnt := raftpb.Entry{ - Term: 1, - Index: 1, - Data: EncodeRaftCommand( - // It would be nice to have an "even more invalid" command here but it - // turns out that DecodeRaftCommand "handles" errors via panic(). - EntryEncodingStandardWithAC, "foobarzz", []byte("definitely not a protobuf"), - ), +func TestNewEntry(t *testing.T) { + // TODO(replication): Add more cases. + testcases := map[string]struct { + data []byte + expectEmpty bool + expectErr bool + }{ + // Proposed by Raft on leader change. + "empty entry": {data: nil, expectEmpty: true}, + // Proposed by CRDB on unquiescence. + "empty payload": { + data: EncodeRaftCommand(EntryEncodingStandardWithoutAC, "00000000", nil), + expectEmpty: true, + }, + "invalid": { + data: EncodeRaftCommand(EntryEncodingStandardWithAC, "00000000", []byte("not a protobuf")), + expectErr: true, + }, + } + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + ent, err := NewEntry(raftpb.Entry{ + Term: 1, + Index: 1, + Data: tc.data, + }) + if tc.expectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + // Clear out the passed Raft entry, and only assert on the decoded entry. + require.NotNil(t, ent) + ent.Entry = raftpb.Entry{} + if tc.expectEmpty { + require.Zero(t, *ent) + } else { + require.NotZero(t, *ent) + } + }) } - ent, err := NewEntry(invalidEnt) - require.Error(t, err) // specific error doesn't matter - require.Zero(t, ent) }