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: only return range info when ClientRangeInfo set #102478

Merged
merged 1 commit into from
May 12, 2023

Conversation

erikgrinaker
Copy link
Contributor

Previously, passing an empty ClientRangeInfo would always generate and return range info, because the zero values are always considered stale. An exception was made for lease requests, since these bypass DistSender and never use ClientRangeInfo.

This patch instead only returns range info when a non-empty ClientRangeInfo is passed, with a version gate for 23.1 compatibility.

Epic: none
Release note: None

@erikgrinaker erikgrinaker requested a review from a team as a code owner April 27, 2023 19:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor Author

See prior discussion in #100432.

Going to let this sit for a little while to get the previous PR some baking time before the backport.

@erikgrinaker erikgrinaker force-pushed the client-range-info-empty branch from 8d4a2fb to 8b503dc Compare May 8, 2023 10:47
@erikgrinaker
Copy link
Contributor Author

#100432 has been baking for a couple of weeks now and the backport will merge shortly, so I think we can review this now @arulajmani.

@andrewbaptist
Copy link
Collaborator

I'll let Arul review/approve this one.

@erikgrinaker erikgrinaker removed the request for review from andrewbaptist May 10, 2023 21:05
Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for coming back and cleaning this up! :lgtm:

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)


pkg/kv/kvserver/replica_send.go line 322 at r1 (raw file):

		}
	} else if ba.IsSingleRequestLeaseRequest() {
		// Remove this branch when 23.1 support is dropped.

nit: want to adopt the TODO for this?

@erikgrinaker erikgrinaker force-pushed the client-range-info-empty branch from 8b503dc to bd11529 Compare May 12, 2023 11:09
Previously, passing an empty `ClientRangeInfo` would always generate and
return range info, because the zero values are always considered stale.
An exception was made for lease requests, since these bypass DistSender
and never use `ClientRangeInfo`.

This patch instead only returns range info when a non-empty
`ClientRangeInfo` is passed, with a version gate for 23.1 compatibility.

Epic: none
Release note: None
@erikgrinaker erikgrinaker force-pushed the client-range-info-empty branch from bd11529 to 15e23d3 Compare May 12, 2023 11:11
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR! There was a subtle bug here: LeaseSpeculative() would return false if the token did not have a lease. This meant that we wouldn't request cache info when we had a range descriptor with generation and no lease. Updated the code to be obviously correct by checking the exact values we're putting into the struct.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)


pkg/kv/kvserver/replica_send.go line 322 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: want to adopt the TODO for this?

Done.

@erikgrinaker
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 12, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented May 12, 2023

Build succeeded:

@craig craig bot merged commit 2e2e5c1 into cockroachdb:master May 12, 2023
@erikgrinaker erikgrinaker deleted the client-range-info-empty branch May 30, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants