Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426763317
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
Quartz's DailyCalendar along with CronCalendar allows `daily excluded,
days of the week/month excluded, etc.`
I've updated the config and implemented it, looking forward for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426755281
##
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/MetricsCollectionAndEvaluationTest.java:
##
@@ -599,6 +602,73 @@ public void testMetricCollectionDuringStabilization()
throws Exception {
assertEquals(2, stateStore.getCollectedMetrics(context).size());
}
+@Test
+public void testMetricCollectionDuringForbidden() throws Exception {
Review Comment:
I want to test this config along with config STABILIZATION_INTERVAL and
METRICS_WINDOW, so put it here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426752508
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
Review Comment:
deleted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426748818
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/JobAutoScalerImpl.java:
##
@@ -96,6 +97,13 @@ public void scale(Context ctx) throws Exception {
return;
}
+if (!AutoScalerUtils.verifyForbidPeriods(ctx.getConfiguration())) {
Review Comment:
removed to `DefaultValidator`
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -114,6 +115,7 @@ public CollectedMetricHistory updateMetrics(
var topology = getJobTopology(ctx, stateStore, jobDetailsInfo);
var stableTime =
jobUpdateTs.plus(conf.get(AutoScalerOptions.STABILIZATION_INTERVAL));
final boolean isStabilizing = now.isBefore(stableTime);
+final boolean isForbidding = AutoScalerUtils.inForbidPeriod(conf, now);
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426747605
##
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/utils/AutoScalerUtilsTest.java:
##
@@ -56,4 +60,30 @@ public void testVertexExclusion() {
Set.of(v1.toString(), v2.toString(), v3.toString()),
new HashSet<>(conf.get(AutoScalerOptions.VERTEX_EXCLUDE_IDS)));
}
+
+@Test
+public void testForbidPeriods() {
Review Comment:
renamed to `testExcludedPeriods()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426564377
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
Yes, if the cron-based notation allows time ranges that would be perfect. We
could just use UTC and document this for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1425382922
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
I checked the pr
https://github.com/apache/flink-kubernetes-operator/pull/656 which introduced
`class CronExpression` from log4j-core and log4j-core copied from Quartz.
1. So can we set the `excluded periods` config to be a cron expression
string and use it's `isSatisfiedBy(final Date date)` method to judge whether in
excluded periods
2. Since standardized cron string doesn't contain a timeZone, should we
just use the default timeZone or some other way to pass it in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1424304876
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
Sounds good, we already use Quartz for `periodic.checkpoint.interval`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1423903323
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
I've seached several time scheduler framework, `Quartz Scheduler` seems to
be feasible.
1. It supports `certain time of day` `certain days of the week` `certain
days of the month` , etc.
https://github.com/quartz-scheduler/quartz/blob/main/docs/introduction.adoc#job-scheduling
2. Here is the implementation:
https://github.com/quartz-scheduler/quartz/blob/a5c4d27e963f51097f9b2777489d310a88897ca4/quartz/src/main/java/org/quartz/Calendar.java#L21-L41
https://github.com/quartz-scheduler/quartz/blob/a5c4d27e963f51097f9b2777489d310a88897ca4/quartz/src/main/java/org/quartz/impl/calendar/CronCalendar.java#L10-L26
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1851810404 > I think we should have a small discussion somewhere (here or the ML) about this feature. Why did you select this specific config syntax? Any alternatives? > > cc @mxm > Thanks @flashJd! This is a useful feature. Let's discuss on the mailing list on the syntax for the exclude periods, as @gyfora suggested. Thanks for the review, the specific config syntax is just a simple implementation for config forbidden periods in a day, it sounds reasonable `we would use some available standard for time spans` and `What about different times at different days of the week / month?` As I just subscribed to the flink dev mailing list, I can't receive the jira email associated with this pr which is sent yesterday and reply to it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1851713592 > I think we should have a small discussion somewhere (here or the ML) about this feature. Why did you select this specific config syntax? Any alternatives? > > cc @mxm -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
flashJd closed pull request #728: [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day URL: https://github.com/apache/flink-kubernetes-operator/pull/728 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1422946292
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
This was also my first thought seeing this. Ideally, we would use some
available standard for time spans. I think this also has to include the time
zone.
What about different times at different days of the week / month?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33795] Add new config to forbid autoscaling in certain periods of a day [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1422702163
##
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/utils/AutoScalerUtilsTest.java:
##
@@ -56,4 +60,30 @@ public void testVertexExclusion() {
Set.of(v1.toString(), v2.toString(), v3.toString()),
new HashSet<>(conf.get(AutoScalerOptions.VERTEX_EXCLUDE_IDS)));
}
+
+@Test
+public void testForbidPeriods() {
Review Comment:
Should be `Forbidden`
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/JobAutoScalerImpl.java:
##
@@ -96,6 +97,13 @@ public void scale(Context ctx) throws Exception {
return;
}
+if (!AutoScalerUtils.verifyForbidPeriods(ctx.getConfiguration())) {
Review Comment:
Config validation should be in the validator module, not here.
##
flink-autoscaler/src/test/java/org/apache/flink/autoscaler/MetricsCollectionAndEvaluationTest.java:
##
@@ -599,6 +602,73 @@ public void testMetricCollectionDuringStabilization()
throws Exception {
assertEquals(2, stateStore.getCollectedMetrics(context).size());
}
+@Test
+public void testMetricCollectionDuringForbidden() throws Exception {
Review Comment:
I think this test should be part of the `JobAutoscalerImplTest` and then it
can be simplified a lot.
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
Review Comment:
deprecated key should not be added
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,15 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> FORBID_PERIOD =
+autoScalerConfig("forbid.periods")
+.stringType()
+.asList()
+.defaultValues()
+
.withDeprecatedKeys(deprecatedOperatorConfigKey("forbid.periods"))
+.withDescription(
+"A (semicolon-separated) list of certain times of
the day during which autoscaling is forbidden,
10:00:00-11:00:00;21:30:00-22:30:00 for example");
Review Comment:
Where did you get this syntax from? What are the potential alternatives?
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -114,6 +115,7 @@ public CollectedMetricHistory updateMetrics(
var topology = getJobTopology(ctx, stateStore, jobDetailsInfo);
var stableTime =
jobUpdateTs.plus(conf.get(AutoScalerOptions.STABILIZATION_INTERVAL));
final boolean isStabilizing = now.isBefore(stableTime);
+final boolean isForbidding = AutoScalerUtils.inForbidPeriod(conf, now);
Review Comment:
Everywhere the word `Forbid` should be replaced with `Forbidden` or
`Blocked` to be correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
