kvserver: reorder leasePostApply on snapshot application#60634
kvserver: reorder leasePostApply on snapshot application#60634craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 4 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/kv/kvserver/replica_proposal.go, line 295 at r1 (raw file):
} type leaseJumpOption bool
Could you give this a comment?
pkg/kv/kvserver/replica_proposal.go, line 315 at r2 (raw file):
// not be aware of other lease changes that happened before the snapshot was // generated. This method thus tolerates prevLease being "stale" when // allowLeaseJump is passed. It's also
Sentence was cut off.
pkg/kv/kvserver/replica_proposal.go, line 320 at r2 (raw file):
// newLease is the same as prevLease. This allows leasePostApplyLocked to be // called for some of its side-effects even if the lease in question has // otherwise already been applied to the range.
Do you think this is better than just saying that the method supports prevLease == newLease and that such situations are idempotent? We still rely on this behavior in applySnapshot, because the lease may not have changed.
My vote would be to get rid of the newLease == nil handling.
pkg/kv/kvserver/replica_proposal.go, line 324 at r2 (raw file):
ctx context.Context, prevLease *roachpb.Lease, newLease *roachpb.Lease, jumpOpt leaseJumpOption, ) { if newLease == nil {
If you want to keep this, give this a comment as well.
pkg/kv/kvserver/replica_raftstorage.go, line 986 at r2 (raw file):
r.store.mu.Unlock() // Inform the concurrency manager that this replica just applied a snapshot.
Pull this down below leasePostApplyLocked as well.
c0ae546 to
19d212c
Compare
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_proposal.go, line 295 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you give this a comment?
done
pkg/kv/kvserver/replica_proposal.go, line 315 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Sentence was cut off.
done
pkg/kv/kvserver/replica_proposal.go, line 320 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do you think this is better than just saying that the method supports
prevLease == newLeaseand that such situations are idempotent? We still rely on this behavior inapplySnapshot, because the lease may not have changed.My vote would be to get rid of the
newLease == nilhandling.
done
pkg/kv/kvserver/replica_proposal.go, line 324 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
If you want to keep this, give this a comment as well.
gone
nvb
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4.
Reviewable status:
pkg/kv/kvserver/replica_proposal.go, line 323 at r4 (raw file):
// allowLeaseJump is passed. prevLease can also be the same as newLease; see below. // // newLease represents the lease being applied. Can be the same as prevLease
nit: missing period.
Release note: None
The snapshot application code was weird - it called leasePostApplyLocked() *before* doing r.mu.state = snapshot.state. This is different from the way it's called when a lease applies regularly through a raft command, where most of the command's effects on the replica state have already been applied (hence the "Post" in the name). This patch reorders things on snapshot application such that r.mu.state is updated before the leasePostApplyLocked call. Besides being sane, this comes in anticipation of leasePostApplyLocked using more elements of the replica state, and needing those to be up to date. Release note: None
19d212c to
4a22463
Compare
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_proposal.go, line 323 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: missing period.
done
pkg/kv/kvserver/replica_raftstorage.go, line 986 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Pull this down below
leasePostApplyLockedas well.
done
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
The snapshot application code was weird - it called
leasePostApplyLocked() before doing r.mu.state = snapshot.state. This
is different from the way it's called when a lease applies regularly
through a raft command, where most of the command's effects on the
replica state have already been applied (hence the "Post" in the name).
This patch reorders things on snapshot application such that r.mu.state
is updated before the leasePostApplyLocked call.
Besides being sane, this comes in anticipation of leasePostApplyLocked
using more elements of the replica state, and needing those to be up to
date.
Release note: None