kv: remove broken attempt to reject lease acquisitions on draining nodes #87885
Conversation
|
This change is |
|
I thought that this log line was useful in cases where draining nodes spuriously re-acquire leases. Do you think we can still keep this log line around? Do you think there's a better way to have this observability? |
nvanbenschoten
force-pushed
the
nvanbenschoten/leaseReqDraining2
branch
from
ef2d7e9 to
fafb80e
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)
pkg/kv/kvserver/replica_range_lease.go line 824 at r1 (raw file):
Previously, aayushshah15 (Aayush Shah) wrote…
I thought that this log line was useful in cases where draining nodes spuriously re-acquire leases. Do you think we can still keep this log line around? Do you think there's a better way to have this observability?
Good point. I unified this with the below-Raft lease acquisition logging.
pkg/kv/kvserver/replica_proposal.go
Outdated
| // 2. the new leaseholder is draining | ||
| // 3. verbose logging is enabled | ||
| epochLeaseChangingHands := newLease.Type() == roachpb.LeaseEpoch && leaseChangingHands | ||
| if epochLeaseChangingHands || r.store.IsDraining() || log.V(1) { | ||
| log.VEventf(ctx, 1, "new range lease %s following %s", newLease, prevLease) |
There was a problem hiding this comment.
Should this be a log.Infof instead?
Related to cockroachdb#83261. This commit removes "protection" that avoided lease acquisitions on draining nodes. This protection had already been effectively disabled by acc1ad1, which allowed Raft leaders to bypass the check. As the comment here (added in 5ffaa9e) explained, Raft followers are already unable to acquire the lease. If leaders bypass the check and follower (and candidates) don't need it, the check is useless, so we remove it. The commit also removes `TestReplicaDrainLease`, which was supposed to test this behavior. We remove the test not because it started failing after the change, but because it did not. It must not have been testing anything real anymore after acc1ad1. Release justification: low risk change related to release blocker. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/leaseReqDraining2
branch
from
fafb80e to
d89a7e4
Compare
|
TFTR! bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Related to #83261.
This commit removes "protection" that avoided lease acquisitions on draining nodes. This protection had already been effectively disabled by acc1ad1, which allowed Raft leaders to bypass the check. As the comment here (added in 5ffaa9e) explained, Raft followers are already unable to acquire the lease. If leaders bypass the check and follower (and candidates) don't need it, the check is useless, so we remove it.
The commit also removes
TestReplicaDrainLease, which was supposed to test this behavior. We remove the test not because it started failing after the change, but because it did not. It must not have been testing anything real anymore after acc1ad1.Release justification: low risk change related to release blocker.
Release note: None