Skip to content

Commit

Permalink
kvserver: remove weird v1-proposals code in propose
Browse files Browse the repository at this point in the history
So glad to see this go.
  • Loading branch information
tbg committed Jul 13, 2023
1 parent 2b39f1a commit 22bbef1
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 75 deletions.
54 changes: 7 additions & 47 deletions pkg/kv/kvserver/replica_application_result.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,13 @@ func (r *Replica) prepareLocalResult(ctx context.Context, cmd *replicatedCmd) {
}
}
if pErr == nil { // since we might have injected an error
if useReproposalsV2 {
pErr = kvpb.NewError(r.tryReproposeWithNewLeaseIndexV2(ctx, cmd))
if pErr == nil {
// Avoid falling through below. We managed to repropose, but this
// proposal is still erroring out. We don't want to assign to
// localResult. If there is an error though, we do fall through into
// the existing tangle of correct but unreadable handling below.
return
}
} else {
pErr = r.tryReproposeWithNewLeaseIndex(ctx, cmd)
pErr = kvpb.NewError(r.tryReproposeWithNewLeaseIndexV2(ctx, cmd))
if pErr == nil {
// Avoid falling through below. We managed to repropose, but this
// proposal is still erroring out. We don't want to assign to
// localResult. If there is an error though, we do fall through into
// the existing tangle of correct but unreadable handling below.
return
}
}

Expand Down Expand Up @@ -400,42 +396,6 @@ func (r *Replica) tryReproposeWithNewLeaseIndexV2(
return nil
}

// tryReproposeWithNewLeaseIndex is used by prepareLocalResult to repropose
// commands that have gotten an illegal lease index error, and that we know
// could not have applied while their lease index was valid (that is, we
// observed all applied entries between proposal and the lease index becoming
// invalid, as opposed to skipping some of them by applying a snapshot).
//
// It is not intended for use elsewhere and is only a top-level function so that
// it can avoid the below_raft_protos check. Returns a nil error if the command
// has already been successfully applied or has been reproposed here or by a
// different entry for the same proposal that hit an illegal lease index error.
func (r *Replica) tryReproposeWithNewLeaseIndex(
ctx context.Context, cmd *replicatedCmd,
) *kvpb.Error {
// Note that we don't need to validate anything about the proposal's
// lease here - if we got this far, we know that everything but the
// index is valid at this point in the log.
p := cmd.proposal
if p.applied || cmd.Cmd.MaxLeaseIndex != p.command.MaxLeaseIndex {
// If the command associated with this rejected raft entry already
// applied then we don't want to repropose it. Doing so could lead
// to duplicate application of the same proposal.
//
// Similarly, if the command associated with this rejected raft
// entry has a different (larger) MaxLeaseIndex than the one we
// decoded from the entry itself, the command must have already
// been reproposed (this can happen if there are multiple copies
// of the command in the logs; see TestReplicaRefreshMultiple).
// We must not create multiple copies with multiple lease indexes,
// so don't repropose it again. This ensures that at any time,
// there is only up to a single lease index that has a chance of
// succeeding in the Raft log for a given command.
return nil
}
return r.tryReproposeWithNewLeaseIndexShared(ctx, cmd.proposal)
}

func (r *Replica) tryReproposeWithNewLeaseIndexShared(
ctx context.Context, p *ProposalData,
) *kvpb.Error {
Expand Down
33 changes: 5 additions & 28 deletions pkg/kv/kvserver/replica_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,34 +356,11 @@ func (r *Replica) propose(
) (pErr *kvpb.Error) {
defer tok.DoneIfNotMoved(ctx)

// If an error occurs reset the command's MaxLeaseIndex to its initial value.
// Failure to propose will propagate to the client. An invariant of this
// package is that proposals which are finished carry a raft command with a
// MaxLeaseIndex equal to the proposal command's max lease index.
defer func(prev kvpb.LeaseAppliedIndex) {
if useReproposalsV2 {
// The following poorly understood code is not necessary in V2 since
// we never mutate MaxLeaseIndex and don't hit propose() twice for
// the same *ProposalData.
return
}
if pErr != nil {
p.command.MaxLeaseIndex = prev
}
}(p.command.MaxLeaseIndex)

if !useReproposalsV2 {
// Make sure the maximum lease index is unset. This field will be set in
// propBuf.Insert and its encoded bytes will be appended to the encoding
// buffer as a MaxLeaseFooter.
p.command.MaxLeaseIndex = 0
} else {
if p.command.MaxLeaseIndex > 0 {
// TODO: there are a number of other fields that should still be unset.
// Verify them all. Some architectural improvements where we pass in a
// subset of ProposalData and then complete it here would be even better.
return kvpb.NewError(errors.AssertionFailedf("MaxLeaseIndex is set: %+v", p))
}
if p.command.MaxLeaseIndex > 0 {
// TODO: there are a number of other fields that should still be unset.
// Verify them all. Some architectural improvements where we pass in a
// subset of ProposalData and then complete it here would be even better.
return kvpb.NewError(errors.AssertionFailedf("MaxLeaseIndex is set: %+v", p))
}

if crt := p.command.ReplicatedEvalResult.ChangeReplicas; crt != nil {
Expand Down

0 comments on commit 22bbef1

Please sign in to comment.