-
Notifications
You must be signed in to change notification settings - Fork 720
feat(frontend): support sink backfill rate limit #24075
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces support for separate backfill rate limiting on sinks by decoupling ThrottleTarget (what to throttle) from ThrottleType (how to throttle). This allows users to independently control both the sink execution rate and the sink backfill rate.
Key Changes:
- Introduced
ThrottleTypeenum to distinguish between DML, Backfill, Source, and Sink rate limit types - Added
ALTER SINK SET BACKFILL_RATE_LIMITSQL syntax support - Updated all executors to check
throttle_typebefore applying rate limits - Modified RPC interfaces to accept both
throttle_targetandthrottle_typeparameters
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/meta.proto | Added ThrottleType enum and updated ApplyThrottleRequest to include both target and type |
| proto/stream_plan.proto | Added nested ThrottleType enum in ThrottleMutation and added throttle_type field |
| src/sqlparser/src/ast/ddl.rs | Added SetBackfillRateLimit variant to AlterSinkOperation enum |
| src/sqlparser/src/parser.rs | Added parsing logic for ALTER SINK SET BACKFILL_RATE_LIMIT syntax |
| src/frontend/src/meta_client.rs | Updated apply_throttle method signature to accept both target and type |
| src/frontend/src/test_utils.rs | Updated mock implementation to match new signature |
| src/frontend/src/handler/mod.rs | Added handler routing for sink backfill rate limit operations |
| src/frontend/src/handler/alter_streaming_rate_limit.rs | Updated logic to handle different throttle target/type combinations |
| src/rpc_client/src/meta_client.rs | Updated apply_throttle to pass both target and type parameters |
| src/meta/service/src/stream_service.rs | Updated throttle application logic to match on (type, target) tuples |
| src/meta/service/src/ddl_service.rs | Updated to pass ThrottleType::Source when creating throttle commands |
| src/meta/src/barrier/command.rs | Modified Command::Throttle to include ThrottleType parameter |
| src/meta/src/barrier/info.rs | Updated pattern matching for throttle commands |
| src/meta/src/barrier/context/context_impl.rs | Updated pattern matching for throttle commands |
| src/stream/src/executor/mod.rs | Updated Mutation::Throttle to include throttle_type field and conversion logic |
| src/stream/src/executor/sink.rs | Added throttle type check to only apply sink rate limits |
| src/stream/src/executor/dml.rs | Added throttle type check to only apply DML rate limits |
| src/stream/src/executor/source/source_executor.rs | Added throttle type check to only apply source rate limits |
| src/stream/src/executor/source/source_backfill_executor.rs | Added throttle type check to only apply backfill rate limits |
| src/stream/src/executor/source/iceberg_fetch_executor.rs | Added throttle type check to only apply source rate limits |
| src/stream/src/executor/source/fs_fetch_executor.rs | Added throttle type check to only apply source rate limits |
| src/stream/src/executor/backfill/no_shuffle_backfill.rs | Added throttle type check to only apply backfill rate limits |
| src/stream/src/executor/backfill/arrangement_backfill.rs | Added throttle type check to only apply backfill rate limits |
| src/stream/src/executor/backfill/cdc/cdc_backfill.rs | Added throttle type check to only apply backfill rate limits |
| src/stream/src/executor/backfill/cdc/cdc_backill_v2.rs | Added throttle type check to only apply backfill rate limits |
| src/ctl/src/cmd_impl/throttle.rs | Added logic to infer throttle type from target for backward compatibility |
|
@copilot please submit a pr to fix the review comments. |
8757617 to
3c0155a
Compare
21a6472 to
3a26d39
Compare
3a26d39 to
bb7def5
Compare
|
@copilot please fix the integration test |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: kwannoel <47273164+kwannoel@users.noreply.github.com>
…24111) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: kwannoel <47273164+kwannoel@users.noreply.github.com>
…24190) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: kwannoel <47273164+kwannoel@users.noreply.github.com>
c6712b9 to
bc923fb
Compare
tabVersion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do need some review on rate limit mechanism, distinguish source_rate_limit and backfill_rate_limit are poor designs, TBH. But introducing ThrottleType may have it deeper nested with prev impl.
| select rate_limit from rw_rate_limit | ||
| ---- | ||
| 10000 | ||
| 10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so one for SINK_RATE_LIMIT and one for backfill_rate_limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
| DISTANCE_TYPE_INNER_PRODUCT = 4; | ||
| } | ||
|
|
||
| enum ThrottleType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type -> Object, as you are rate limit database objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refers to the type of rate limit we are applying. Source, backfill, sink, dml these are all different types of rate limits.
| /// The type of throttle to apply | ||
| #[clap(long, value_enum, required = true)] | ||
| throttle_type: ThrottleTypeArg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also if using id, there is no need for ThrottleTypeArg. Id is unique in the cluster, using id can find the object and determine the throttle_type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Id is not specified at the executor level, it's done at the fragment level in the current implementation. In such a scenario, consider when sink executor and scan executor are collocated in the same fragment. When altering either rate limit (sink or backfill), both rate limits will be changed, since the provided id is fragment id. You can see that the slt test was changed, because this was incorrect behaviour that existed due to the current design.
| ThrottleTarget throttle_target = 1; | ||
| common.ThrottleType throttle_type = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think ThrottleTarget and ThrottleType are orthogonal in concept.
ThrottleType in your design stands for things you want to apply rate limit, but ThrottleTarget also contain similar concepts.
Maybe we can change to ThrottleObject and ThrottleDirection,
eg. ThrottleObject.sink + ThrottleDirection.out = sink_rate_limit and ThrottleObject.sink + ThrottleDirection.in = backfill_rate_limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we support streaming rate limit in the future, direction also becomes ambiguous. streaming and source share the same semantics. Further consider altering a cdc table rate limit. Lets say we specify the direction "in". Are we changing the dml rate limit? Or the source rate limit? Or the backfill rate limit? Decoupling the rate limit type from the target makes this unambiguous.
Chiro11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
ThrottleTargetintoThrottleTargetandThrottleType.ThrottleTyperefers to the type of rate limit we want to apply.ThrottleTargetrefers to the actual object we are targeting (sink, index, fragment, etc...).Checklist
Documentation
Release note