-
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?
Changes from all commits
371bd9c
6f71c85
8b32a59
7528ac6
7ac739a
4c330f7
6cdde0e
4ce4e4e
be204ed
2942352
27bf2cf
81cdd86
23e1216
bc923fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -194,3 +194,11 @@ enum DistanceType { | |
| DISTANCE_TYPE_COSINE = 3; | ||
| DISTANCE_TYPE_INNER_PRODUCT = 4; | ||
| } | ||
|
|
||
| enum ThrottleType { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type -> Object, as you are rate limit database objects
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| THROTTLE_TYPE_UNSPECIFIED = 0; | ||
| THROTTLE_TYPE_DML = 1; | ||
| THROTTLE_TYPE_BACKFILL = 2; | ||
| THROTTLE_TYPE_SOURCE = 3; | ||
| THROTTLE_TYPE_SINK = 4; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -346,21 +346,20 @@ | |
| repeated ObjectDependencies dependencies = 1; | ||
| } | ||
|
|
||
| enum ThrottleTarget { | ||
|
Check failure on line 349 in proto/meta.proto
|
||
| THROTTLE_TARGET_UNSPECIFIED = 0; | ||
| SOURCE = 1; | ||
| MV = 2; | ||
| TABLE_WITH_SOURCE = 3; | ||
| CDC_TABLE = 4; | ||
| TABLE_DML = 5; | ||
| SINK = 6; | ||
| FRAGMENT = 7; | ||
| TABLE = 3; | ||
| SINK = 4; | ||
| FRAGMENT = 5; | ||
| } | ||
|
|
||
| message ApplyThrottleRequest { | ||
| ThrottleTarget kind = 1; | ||
| uint32 id = 2; | ||
| optional uint32 rate = 3; | ||
| ThrottleTarget throttle_target = 1; | ||
|
Check failure on line 359 in proto/meta.proto
|
||
| common.ThrottleType throttle_type = 2; | ||
|
Comment on lines
+359
to
+360
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont think ThrottleTarget and ThrottleType are orthogonal in concept. Maybe we can change to ThrottleObject and ThrottleDirection,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| uint32 id = 3; | ||
| optional uint32 rate = 4; | ||
| } | ||
|
|
||
| message ApplyThrottleResponse { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -506,12 +506,28 @@ enum TestCommands { | |
| enum ThrottleCommands { | ||
| Source(ThrottleCommandArgs), | ||
| Mv(ThrottleCommandArgs), | ||
| Sink(ThrottleCommandArgs), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, clap::ValueEnum)] | ||
| pub enum ThrottleTypeArg { | ||
| Dml, | ||
| Backfill, | ||
| Source, | ||
| Sink, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Args)] | ||
| pub struct ThrottleCommandArgs { | ||
| /// The ID of the object to throttle | ||
| #[clap(long, required = true)] | ||
| id: u32, | ||
| /// The rate limit to apply | ||
| #[clap(long)] | ||
| rate: Option<u32>, | ||
| /// The type of throttle to apply | ||
| #[clap(long, value_enum, required = true)] | ||
| throttle_type: ThrottleTypeArg, | ||
|
Comment on lines
+528
to
+530
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| } | ||
|
|
||
| #[derive(Subcommand, Clone, Debug)] | ||
|
|
@@ -921,6 +937,9 @@ async fn start_impl(opts: CliOpts, context: &CtlContext) -> Result<()> { | |
| Commands::Throttle(ThrottleCommands::Mv(args)) => { | ||
| apply_throttle(context, risingwave_pb::meta::PbThrottleTarget::Mv, args).await?; | ||
| } | ||
| Commands::Throttle(ThrottleCommands::Sink(args)) => { | ||
| apply_throttle(context, risingwave_pb::meta::PbThrottleTarget::Sink, args).await?; | ||
| } | ||
| Commands::Meta(MetaCommands::SetCdcTableBackfillParallelism { | ||
| table_id, | ||
| parallelism, | ||
|
|
||
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