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/rangefeed: always use time-bound iterators for catchup scans #82450

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jun 5, 2022

EDIT: Oops, I completely missed that this had already been defaulted to true in #73473. Got confused by #79727, which I took to mean that it hadn't been flipped yet. This PR essentially just removed the setting then. Sorry for the confusion.


This patch always uses time-bound iterators for rangefeed catchup scans.
Previously, this was controlled by the default-off cluster setting
kv.rangefeed.catchup_scan_iterator_optimization.enabled, which has now
been removed. These have been used in production by several users, who
have seen significant performance improvements without reports of any
issues.

Resolves #79728.
Touches #82454.

Release note (performance improvement): Changefeed catchup scans now use
time-bound iterators, which improves their performance by avoiding
accessing data that is outside the catchup scan time interval.
Previously, this was controlled by the default-off cluster setting
kv.rangefeed.catchup_scan_iterator_optimization.enabled, which has now
been removed (it is in effect always enabled).

@erikgrinaker erikgrinaker requested review from stevendanna, samiskin and a team June 5, 2022 10:29
@erikgrinaker erikgrinaker requested a review from a team as a code owner June 5, 2022 10:29
@erikgrinaker erikgrinaker self-assigned this Jun 5, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This patch always uses time-bound iterators for rangefeed catchup scans.
Previously, this was controlled by the default-off cluster setting
`kv.rangefeed.catchup_scan_iterator_optimization.enabled`, which has now
been removed. These have been used in production by several users, who
have seen significant performance improvements without reports of any
issues.

Release note (performance improvement): Changefeed catchup scans now use
time-bound iterators, which improves their performance by avoiding
accessing data that is outside the catchup scan time interval.
Previously, this was controlled by the default-off cluster setting
`kv.rangefeed.catchup_scan_iterator_optimization.enabled`, which has now
been removed (it is in effect always enabled).
@erikgrinaker
Copy link
Contributor Author

Thanks!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Jun 6, 2022

Build succeeded:

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 6, 2022

Oops, I completely missed that this had already been defaulted to true in #73473. Got confused by #79727, which I took to mean that it hadn't been flipped yet. This PR essentially just removed the setting then. Sorry for the confusion.

@shermanCRL
Copy link
Contributor

shermanCRL commented Jun 6, 2022

Should we backport to v22.1? It is an API change, which argues against. That said, from a support perspective, I’d love to remove yet another “check this setting...”. Answered below.

@erikgrinaker
Copy link
Contributor Author

Should we backport to v22.1? It is an API change, which argues against. That said, from a support perspective, I’d love to remove yet another “check this setting...”.

I'd leave it in for 22.1, just to have an escape hatch. Just discovered that TBIs may omit inline values, and I don't know yet whether any of the new span config stuff in 22.1 (which relies on rangefeeds) can run across inline values, which might be problematic: #82453 (comment).

@erikgrinaker erikgrinaker deleted the rangefeed-catchup-tbi branch June 8, 2022 11:44
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.

changefeedccl, settings: retire kv.rangefeed.catchup_scan_iterator_optimization.enabled
4 participants