kv: return empty LeaseStatus from leaseGoodToGoRLocked during follower reads #60901
Conversation
…r reads Fixes cockroachdb#60830. This was broken in c44b357 and started causing problems after 0668efb. A non-empty LeaseStatus during follower reads was tripping up logic that assumed that a VALID LeaseStatus indicated that the serving replica was the leaseholder. This assumption used to hold, but unintentionally did not anymore. Returning an empty LeaseStatus on follower reads instead of expanding the meaning of a VALID LeaseStatus to include follower reads seems like the right change. This is because we want to get to a place where the lease is only checked if a replica does not have a sufficient closed timestamp, so follower reads should never even need to consult the lease - see cockroachdb#57992.
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
There was a problem hiding this comment.
These flakes are extremely frequent. I'm going to bors this for you. Hope that's okay by you.
bors r=ajwerner
Reviewable status:
There was a problem hiding this comment.
oh I just saw your note that this may not be the root cause, err, my bad. Feel free to bors r- but this still seems reasonable.
Reviewable status:
Perfectly fine by me. Thanks for the review. I do think this is the root cause - 0668efb just revealed the issue because it made follower reads so much more consistent. |
|
Build failed: |
|
Let’s try that again now that the revert landed. bors r+ |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
Fixes #60830.
This was broken in c44b357 and started causing problems after 0668efb.
A non-empty LeaseStatus during follower reads was tripping up logic that
assumed that a VALID LeaseStatus indicated that the serving replica was
the leaseholder. This assumption used to hold, but unintentionally did
not anymore.
Returning an empty LeaseStatus on follower reads instead of expanding
the meaning of a VALID LeaseStatus to include follower reads seems like
the right change. This is because we want to get to a place where the
lease is only checked if a replica does not have a sufficient closed
timestamp, so follower reads should never even need to consult the
lease - see #57992.