Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: unquiesce proposal always errors #100400

Closed
erikgrinaker opened this issue Apr 1, 2023 · 1 comment · Fixed by #100401
Closed

kvserver: unquiesce proposal always errors #100400

erikgrinaker opened this issue Apr 1, 2023 · 1 comment · Fixed by #100401
Assignees
Labels
branch-master Failures and bugs on the master branch. branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-performance Perf of queries or internals. Solution not expected to change functional behavior. GA-blocker

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Apr 1, 2023

When we unquiesce a Raft group we submit an empty proposal:

data := raftlog.EncodeRaftCommand(raftlog.EntryEncodingStandardWithoutAC, makeIDKey(), nil)

However, this proposal always results in an error, because its ProposerLeaseSequence is always 0:

nlhe := kvpb.NewNotLeaseHolderError(
*replicaState.Lease, 0 /* proposerStoreID */, replicaState.Desc,
fmt.Sprintf(
"stale proposal: command was proposed under lease #%d but is being applied "+
"under lease: %s", raftCmd.ProposerLeaseSequence, replicaState.Lease))

The construction of such errors has a non-negligible cost. During expiration-based lease testing with eager lease extensions, on an idle cluster with 20.000 lease extensions every 3 seconds, these accounted for 10% of CPU usage.

There is a related TODO here:

// 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")

We should treat these proposals as noops, similarly to what we already do here:

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).
return ForcedErrResult{
LeaseIndex: leaseIndex,
Rejection: ProposalRejectionPermanent,
ForcedError: noopOnEmptyRaftCommandErr,
}
}

This code was introduced recently in #76126, so it may be a new issue in 23.1. I'm therefore marking it as a GA blocker.

Jira issue: CRDB-26411

@erikgrinaker erikgrinaker added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-performance Perf of queries or internals. Solution not expected to change functional behavior. branch-master Failures and bugs on the master branch. GA-blocker T-kv-replication branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 labels Apr 1, 2023
@erikgrinaker erikgrinaker self-assigned this Apr 1, 2023
@blathers-crl
Copy link

blathers-crl bot commented Apr 1, 2023

cc @cockroachdb/replication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-performance Perf of queries or internals. Solution not expected to change functional behavior. GA-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant