-
Notifications
You must be signed in to change notification settings - Fork 498
[FLINK-36836][Autoscaler] Supports config the upper and lower limits of target utilization #921
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -89,13 +89,17 @@ private static ConfigOptions.OptionBuilder autoScalerConfig(String key) { | |
| + "seconds suffix, daily expression's formation is startTime-endTime, such as 9:30:30-10:50:20, when exclude from 9:30:30-10:50:20 in Monday and Thursday " | ||
| + "we can express it as 9:30:30-10:50:20 && * * * ? * 2,5"); | ||
|
|
||
| public static final ConfigOption<Double> TARGET_UTILIZATION = | ||
| autoScalerConfig("target.utilization") | ||
| public static final ConfigOption<Double> UTILIZATION_TARGET = | ||
| autoScalerConfig("utilization.target") | ||
| .doubleType() | ||
| .defaultValue(0.7) | ||
| .withFallbackKeys(oldOperatorConfigKey("target.utilization")) | ||
| .withDeprecatedKeys(autoScalerConfigKey("target.utilization")) | ||
| .withFallbackKeys( | ||
| oldOperatorConfigKey("utilization.target"), | ||
| oldOperatorConfigKey("target.utilization")) | ||
| .withDescription("Target vertex utilization"); | ||
|
|
||
| @Deprecated | ||
| public static final ConfigOption<Double> TARGET_UTILIZATION_BOUNDARY = | ||
| autoScalerConfig("target.utilization.boundary") | ||
| .doubleType() | ||
|
|
@@ -104,6 +108,20 @@ private static ConfigOptions.OptionBuilder autoScalerConfig(String key) { | |
| .withDescription( | ||
| "Target vertex utilization boundary. Scaling won't be performed if the processing capacity is within [target_rate / (target_utilization - boundary), (target_rate / (target_utilization + boundary)]"); | ||
|
|
||
| public static final ConfigOption<Double> UTILIZATION_MAX = | ||
| autoScalerConfig("utilization.max") | ||
| .doubleType() | ||
| .noDefaultValue() | ||
| .withFallbackKeys(oldOperatorConfigKey("utilization.max")) | ||
| .withDescription("Max vertex utilization"); | ||
|
|
||
| public static final ConfigOption<Double> UTILIZATION_MIN = | ||
| autoScalerConfig("utilization.min") | ||
| .doubleType() | ||
| .noDefaultValue() | ||
| .withFallbackKeys(oldOperatorConfigKey("utilization.min")) | ||
| .withDescription("Min vertex utilization"); | ||
|
Comment on lines
+111
to
+123
Member
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. These 2 options have no default value, I'm not sure whether it's expected. If the Usually, when a new option is introduced, a default value is also designed. When deprecated options are removed in subsequent versions, we only need to remove unnecessary compatibility code.
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.
Member
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. As I understand, we will only keep the following three options in the end, and make it compatible with Do you mean we keep all 4 options in the end?
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 think we should remove the boundary at the end (deprecate now), but still min/max should not have fixed defaults as the default would be derived from the current target. |
||
|
|
||
| public static final ConfigOption<Duration> SCALE_DOWN_INTERVAL = | ||
| autoScalerConfig("scale-down.interval") | ||
| .durationType() | ||
|
|
||
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.
Do we have to change this well-established key? This might confuse users, even though we have a fallback.
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 understand its cleaner, but perhaps we can simply add the new keys and leave this unchanged?
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 think methed
withFallbackKeysgood enoughWDYT?
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 think in the long term it's preferable to have consistent well named keys. I think the fallback keys should be enough. I would expect that users will gravitate towards the new min/max options and at that point this will make a lot of sense for everyone.