raftlog: correctly decode empty commands

[erikgrinaker]

When unquiescing a Raft group, an empty command is proposed to wake the Raft leader. During application, empty entries are considered noops, but only if they don't have a command ID. However, Raft entry decoding would preserve the command ID of an empty payload. CheckForcedErr() thus no longer considered this a noop, instead returning an error because the ProposerLeaseSequence of 0 was older than the current lease. Construction and processing of this error had a non-negligible cost when unquiescing a large number of ranges.

This patch makes sure such empty commands are decoded as empty entries, without a command ID, and thus considered noops.

Resolves #100400.

Epic: none
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[erikgrinaker]

An alternative here is for either Entry.load() or CheckForcedErr() to handle empty payloads as a noop. I don't have a strong opinion either way -- it would buy us a command ID, which would allow us to disambiguate these from the empty proposals that Raft itself submits, at the cost of a few additional bytes, but otherwise I don' t think we really care.

[erikgrinaker]

Note to self: check for any backwards compatibility concerns with 22.2 here.

[erikgrinaker]

v22.2 uses an empty payload rather than an empty entry:

cockroach/pkg/kv/kvserver/replica_raft_quiesce.go

Lines 91 to 92 in 43a37d5

	data := kvserverbase.EncodeRaftCommand(kvserverbase.RaftVersionStandard, makeIDKey(), nil)
	_ = r.mu.internalRaftGroup.Propose(data)

This empty payload was handled here:

cockroach/pkg/kv/kvserver/replica_application_cmd.go

Lines 224 to 231 in 5819d7a

	d.idKey, encodedCommand = kvserverbase.DecodeRaftCommand(e.Data)
	// An empty command is used to unquiesce a range and wake the
	// leader. Clear commandID so it's ignored for processing.
	if len(encodedCommand) == 0 {
	d.idKey = ""
	} else if err := protoutil.Unmarshal(encodedCommand, &d.raftCmd); err != nil {
	return wrapWithNonDeterministicFailure(err, "while unmarshaling entry")
	}

I've therefore updated this PR to retain the previous encoding (with command ID), and instead decode empty payloads into empty entries, for backwards compatibility with 22.2 entries.

[pav-kv]

Ended up nits-only, so feel free to skip them.

[pav-kv]

An alternative here is for either Entry.load() or CheckForcedErr() to handle empty payloads as a noop. I don't have a strong opinion either way -- it would buy us a command ID, which would allow us to disambiguate these from the empty proposals that Raft itself submits, at the cost of a few additional bytes, but otherwise I don' t think we really care.

Maybe all of these would be better indicated with some explicit enum, but it would be a bigger change.

[erikgrinaker]

Maybe all of these would be better indicated with some explicit enum, but it would be a bigger change.

Yeah, I'm going for a targeted change here, since this needs a 23.1 backport.

[erikgrinaker]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)