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

Revert "rangefeed: unconditionally use rangefeed scheduler" #121164

Merged

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Mar 27, 2024

This reverts 3/4 commits from #114410, only leaving 1dbdbe9 which enabled the scheduler by default.

There was significant code drift in tests, but I think I've been able to address the conflicts.

This revert does not integrate with the changes from #120347, so the legacy rangefeed processor will not use the improved memory accounting. We still shouldn't leak budget allocations, since the processor implementations are entirely independent, but it's worth having a second look to make sure the registry (which is used by both processors) don't make assumptions about this.

Touches #121054.
Epic: none.
Release note: the following release note from #114410 should be reverted/ignored: "the setting kv.rangefeed.scheduler.enabled has been removed, as the rangefeed scheduler is now unconditionally enabled".

@erikgrinaker erikgrinaker self-assigned this Mar 27, 2024
@erikgrinaker erikgrinaker requested review from a team as code owners March 27, 2024 00:04
@erikgrinaker erikgrinaker requested review from DarrylWong and renatolabs and removed request for a team March 27, 2024 00:04
Copy link

blathers-crl bot commented Mar 27, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the rangefeed-revert-unconditional-scheduler branch 2 times, most recently from 91cf8d0 to 662f837 Compare March 27, 2024 12:27
@erikgrinaker
Copy link
Contributor Author

I've run a few roachtests to verify, seems to work fine. I think this should be good to go.

it's worth having a second look to make sure the registry (which is used by both processors) don't make assumptions about this

SharedBudgetAllocation.Use() and Release() handle nil allocations just fine, so it's ok for the legacy processor to not attach allocations to all events.

@wenyihu6
Copy link
Contributor

re: memory accounting part, legacy and scheduled processor seem to handle everything the same way, except for my recent changes (which is just an improvement, no memory leaks without it), so it looks fine.

I will defer the stamp to those who reviewed the original PRs though.

Copy link
Collaborator

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

:lgtm: Why were the reverts in store.go and testing_knobs.go no longer needed?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @erikgrinaker, @nvanbenschoten, @renatolabs, and @wenyihu6)


pkg/kv/kvclient/rangefeed/rangefeed_external_test.go line 69 at r4 (raw file):

func (t feedProcessorType) String() string {
	return fmt.Sprintf("single/scheduler=%t", t.useScheduler)

Should this be mux instead of single?

@erikgrinaker erikgrinaker force-pushed the rangefeed-revert-unconditional-scheduler branch from 662f837 to d8f53d5 Compare March 27, 2024 15:13
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.

It didn't seem useful to revert those changes -- I think we want the scheduler to be responsive to testing knobs regardless.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @nvanbenschoten, @renatolabs, @rharding6373, and @wenyihu6)


pkg/kv/kvclient/rangefeed/rangefeed_external_test.go line 69 at r4 (raw file):

Previously, rharding6373 (Rachael Harding) wrote…

Should this be mux instead of single?

It should, good catch -- thanks!

@erikgrinaker
Copy link
Contributor Author

I'm going to optimistically bors this to get it in before the branch cut, as I only changed a string since the last CI run.

bors r+

@craig craig bot merged commit 87ed0b2 into cockroachdb:master Mar 27, 2024
20 of 22 checks passed
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.

None yet

4 participants