rangefeed: unconditionally use rangefeed scheduler #114410
Conversation
erikgrinaker
requested review from
herkolategan and
DarrylWong
and removed request for
a team
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
erikgrinaker
force-pushed
the
rangefeed-scheduler-default
branch
from
22bc8cb
to
7e8809b
Compare
There was a problem hiding this comment.
Reviewed 9 of 12 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)
pkg/kv/kvserver/rangefeed/processor_test.go line 333 at r2 (raw file):
sch := NewScheduler(SchedulerConfig{ Workers: 1,
do we have tests that spin use multiple workers?
Does it make sense to just throw some metamorphic constant here and for the priority workers below?
|
@erikgrinaker -- what's left for this PR? (other than CI)? |
|
Nothing really, just wanted to wait for 23.2 to firm up a bit first, to make it easier to backport fixes. I'll address your comments and merge this when I have time. |
erikgrinaker
force-pushed
the
rangefeed-scheduler-default
branch
from
7e8809b
to
2cfd5f9
Compare
There was a problem hiding this comment.
You might need to change mixed_version_cdc.go
Thanks, done.
Reviewable status:
pkg/kv/kvserver/rangefeed/processor_test.go line 333 at r2 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
do we have tests that spin use multiple workers?
Does it make sense to just throw some metamorphic constant here and for the priority workers below?
It won't make any difference here. There's only a single range, which can only be processed by a single worker at a time by definition. Multiple workers are only relevant with multiple ranges, and these are covered by integration and end-to-end tests.
erikgrinaker
force-pushed
the
rangefeed-scheduler-default
branch
2 times, most recently
from
5efb944
to
941af2e
Compare
Epic: none Release note (performance improvement): rangefeeds, the infrastructure used for changefeeds, now use a more efficient engine that reduces the number of goroutines and the associated Go scheduler pressure and latency.
erikgrinaker
force-pushed
the
rangefeed-scheduler-default
branch
from
941af2e
to
4075e88
Compare
This patch unconditionally uses the new rangefeed scheduler. This has no mixed-version concerns, since the scheduler is internal to each node and uses the same client protocol. Epic: none Release note (ops change): the setting `kv.rangefeed.scheduler.enabled` has been removed, as the rangefeed scheduler is now unconditionally enabled.
The rangefeed scheduler is now used unconditionally. Epic: none Release note: None
This has no effect for scheduler-based processors, which is now the only processor type. Instead, the `RangeFeedPushTxnsInterval` testing knob is now propagated to the store's rangefeed txn pusher loop. Epic: none Release note: None
erikgrinaker
force-pushed
the
rangefeed-scheduler-default
branch
from
4075e88
to
9e97770
Compare
|
bors r+ |
|
Build succeeded: |
121138: storage,ingest: enable ingestion optimization using flushableIngest r=aadityasondhi a=aadityasondhi In cockroachdb/pebble#3398, we introduced an optimization in pebble where we could use flushableIngests with excises for ingesting SSTs with RangeDels and RangeKeyDels. This patch turns this optimization on using `sstContainsExciseTombstone`. Informs cockroachdb/pebble#3335. Release note: None 121164: Revert "rangefeed: unconditionally use rangefeed scheduler" r=erikgrinaker a=erikgrinaker 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". 121212: security: wrap error when parsing DN r=rafiss a=rafiss Epic: CRDB-34126 Release note: None Co-authored-by: Aaditya Sondhi <20070511+aadityasondhi@users.noreply.github.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
kvserver: enable rangefeed scheduler by default
Touches #113029.
Epic: none
Release note (performance improvement): rangefeeds, the infrastructure used for changefeeds, now use a more efficient engine that reduces the number of goroutines and the associated Go scheduler pressure and latency.
rangefeed: unconditionally use rangefeed scheduler
This patch unconditionally uses the new rangefeed scheduler. This has no mixed-version concerns, since the scheduler is internal to each node and uses the same client protocol.
Epic: none
Release note (ops change): the setting
kv.rangefeed.scheduler.enabledhas been removed, as the rangefeed scheduler is now unconditionally enabled.rangefeed: remove
LegacyProcessorThe rangefeed scheduler is now used unconditionally.
rangefeed: remove
Config.PushTxnsIntervalThis has no effect for scheduler-based processors, which is now the only processor type. Instead, the
RangeFeedPushTxnsIntervaltesting knob is now propagated to the store's rangefeed txn pusher loop.