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.

Sign up for GitHub

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

Jump to bottom

changefeedccl: parallelize event consumption #87994

Merged
merged 2 commits into from
Sep 27, 2022
Merged

changefeedccl: parallelize event consumption #87994

merged 2 commits into from
Sep 27, 2022

Conversation

jayshrivastava
Copy link
Contributor

@jayshrivastava jayshrivastava commented Sep 15, 2022 •
edited
Loading

Fixes: #86902

span: make span.Frontier thread safe

Make span.Frontier thread safe by default.

Release note: None

changefeedccl: parallelize event consumption

Previously, KV events were consumed and processed by changefeed
aggregator using a single, synchronous Go routine. This PR makes
it possible to run up to changefeed.event_consumer_workers
consumers to consume events concurrently. The cluster setting
changefeed.event_consumer_worker_queue_size is added to help
control the number of concurrent events we can have in flight.

Specifying changefeed.event_consumer_workers=0 keeps existing
single threaded implementation.

Release note (enterprise change): This change adds the cluster setting
changefeed.event_consumer_workers which allows changefeeds to
process events concurrently.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jayshrivastava jayshrivastava marked this pull request as ready for review September 15, 2022 15:21
@jayshrivastava jayshrivastava requested a review from a team as a code owner September 15, 2022 15:21
@jayshrivastava jayshrivastava requested review from shermanCRL, miretskiy and a team and removed request for a team September 15, 2022 15:21
@shermanCRL shermanCRL mentioned this pull request Sep 15, 2022
Closed
shermanCRL
shermanCRL reviewed Sep 15, 2022
View reviewed changes
pkg/ccl/changefeedccl/changefeedbase/settings.go Outdated
settings.TenantWritable,
"changefeed.event_consumer_workers",
"the number of workers to use when processing events; 0 disables",
32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the improvement over single-threaded should be pretty evident with 2 workers, maybe let’s do our testing with low numbers to better understand the behavior.

@miretskiy
Copy link
Contributor

You probably need to drop changefeedccl: disable flushing when oom test under race commit since I believe this issue been fixed on the master.

@jayshrivastava jayshrivastava requested a review from a team September 23, 2022 17:08
@jayshrivastava jayshrivastava requested a review from a team as a code owner September 23, 2022 18:20
miretskiy
miretskiy approved these changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

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

Mostly nits; please remember to drop your top commit.

pkg/ccl/changefeedccl/changefeedbase/settings.go Outdated
"changefeed.event_consumer_worker_queue_size",
"if changefeed.event_consumer_workers is enabled, this setting sets the maxmimum number of events"+
"which a worker can buffer",
16,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make that metamorphic... 0 -> 16?
Also.... 16 was rather arbitrary, no?

Copy link
Contributor Author

@jayshrivastava jayshrivastava Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's arbitrary. Making it unbounded is a bit scary. I'll make it metamorphic for 1-16.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out, using 0 for size is great at finding bugs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I'll do it. But, I will look forward to setting it to my scientifically chosen value of 16 in the next month or so 😄

pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/event_processing.go Outdated Show resolved Hide resolved
pkg/ccl/changefeedccl/sink.go Show resolved Hide resolved
@jayshrivastava
span: make span.Frontier thread safe
70c4cbc 
Make `span.Frontier` thread safe by default.

Release note: None
@jayshrivastava
changefeedccl: parallelize event consumption
81116ae 
Previously, KV events were consumed and processed by changefeed
aggregator using a single, synchronous Go routine. This PR makes
it possible to run up to `changefeed.event_consumer_workers`
consumers to consume events concurrently. The cluster setting
`changefeed.event_consumer_worker_queue_size` is added to help
control the number of concurrent events we can have in flight.

Specifying `changefeed.event_consumer_workers=0` keeps existing
single threaded implementation.

Release note (enterprise change): This change adds the cluster setting
`changefeed.event_consumer_workers` which allows changefeeds to
process events concurrently.
@jayshrivastava
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 26, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 27, 2022

Build succeeded:

@craig craig bot merged commit c55586b into cockroachdb:master Sep 27, 2022
@cockroach-teamcity cockroach-teamcity mentioned this pull request Sep 27, 2022
Open
@jayshrivastava
Copy link
Contributor Author

Posting results for reference. 350MB/s -> 700MB/s improvement.

image

Results depend on #88395

@shermanCRL
Copy link
Contributor

I’ll release my plug for lower numbers, perhaps even 2, as default to start with. Of course we're eager to have higher throughput of course, but a lower number will help us to observe and debug, and is likely to minimize surprises.

Customers can always increase the # on our advice, and we can increase defaults in future releases.

@miretskiy
Copy link
Contributor

I don't know... I'd go with 4 or 8; if there are bugs I'd rather see those than try tomake them less likely.

@miretskiy miretskiy mentioned this pull request Sep 28, 2022
Closed
@jayshrivastava jayshrivastava deleted the nprocs-3 branch September 28, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shermanCRL shermanCRL shermanCRL left review comments

@miretskiy miretskiy miretskiy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

changefeedccl/kvevent: implement parallel event consumer
4 participants
@jayshrivastava @cockroach-teamcity @miretskiy @shermanCRL