Re: [PR] [FLINK-33795] Add a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm merged PR #728: 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
1996fanrui commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1436336459
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,21 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> EXCLUDED_PERIODS =
+autoScalerConfig("excluded.periods")
+.stringType()
+.asList()
+.defaultValues()
Review Comment:
It's fine for me.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r143629
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,21 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> EXCLUDED_PERIODS =
+autoScalerConfig("excluded.periods")
+.stringType()
+.asList()
+.defaultValues()
Review Comment:
@1996fanrui change the defaultValue to `noDefaultValue` causes NPE， we have
to add many annoying
notNull judgments, as config `vertex.exclude.ids` also set to
defaultValues(), I referred to it, what do you think.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1436178955
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,21 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> EXCLUDED_PERIODS =
+autoScalerConfig("excluded.periods")
+.stringType()
+.asList()
+.defaultValues()
Review Comment:
Fixed, can you approve the workflow again, thanks.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
1996fanrui commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1436102422
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/config/AutoScalerOptions.java:
##
@@ -72,6 +72,21 @@ private static ConfigOptions.OptionBuilder
autoScalerConfig(String key) {
.withDescription(
"Stabilization period in which no new scaling will
be executed");
+public static final ConfigOption> EXCLUDED_PERIODS =
+autoScalerConfig("excluded.periods")
+.stringType()
+.asList()
+.defaultValues()
Review Comment:
```suggestion
.noDefaultValue()
```
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434919437
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Let's do that in a separate PR.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434917434
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I think it is a good idea to send events for those **once** per
stabilization / metric collection period. We have an event de-duplication
feature to avoid sending them more often.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434780474
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Thanks for the review. A small question, what do you think of the frequent
log printing, e.g. `Stabilizing until ...` and `Metric window not full until
...`, is it worth to put this frequent log to autoscale event handler.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434780474
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Thanks for the review. A small question, what do you think of the frequent
log printing, `Stabilizing until ...` and `Metric window not full until ...`,
is it worth to put this frequent log to autoscale event handler.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434780474
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Thanks for the review. A small question, what do you think of the frequent
log printing, `Stabilizing until ...` and `Metric window not full until ...`,
is it worth to put some frequent log to autoscale event handler.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434766546
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/event/AutoScalerEventHandler.java:
##
@@ -83,12 +92,27 @@ default void handleScalingEvent(
}
static String scalingReport(
-Map scalingSummaries, boolean
scalingEnabled) {
-StringBuilder sb =
-new StringBuilder(
-scalingEnabled
-? SCALING_SUMMARY_HEADER_SCALING_ENABLED
-: SCALING_SUMMARY_HEADER_SCALING_DISABLED);
+Map scalingSummaries,
+boolean scalingEnabled,
+boolean isExcluded,
+Configuration config) {
+StringBuilder sb = new StringBuilder();
+if (!scalingEnabled) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+SCALING_ENABLED.key(),
+false));
+} else if (isExcluded) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+EXCLUDED_PERIODS.key(),
+config.get(EXCLUDED_PERIODS)));
+} else {
+sb.append(SCALING_SUMMARY_HEADER_SCALING_EXECUTION_ENABLED);
+}
Review Comment:
Modify the parameter of method `handleScalingEvent` to replace the
scalingEnabled and scalingBlocked flags via a generic String
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434764455 ## flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java: ## @@ -264,4 +262,21 @@ private static Map getVertexParallelismOverrides( }); return overrides; } + +private boolean blockScalingExecution( Review Comment: Renamed to `checkIfBlockedAndTriggerScalingEvent` -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434220037 ## flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java: ## @@ -264,4 +262,21 @@ private static Map getVertexParallelismOverrides( }); return overrides; } + +private boolean blockScalingExecution( Review Comment: We should rename this to `triggerScalingEvent` or `checkIfBlockedAndTriggerScalingEvent` or add javadocs or something that suggests that the scaling event is triggered inside -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434216202
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/event/AutoScalerEventHandler.java:
##
@@ -83,12 +92,27 @@ default void handleScalingEvent(
}
static String scalingReport(
-Map scalingSummaries, boolean
scalingEnabled) {
-StringBuilder sb =
-new StringBuilder(
-scalingEnabled
-? SCALING_SUMMARY_HEADER_SCALING_ENABLED
-: SCALING_SUMMARY_HEADER_SCALING_DISABLED);
+Map scalingSummaries,
+boolean scalingEnabled,
+boolean isExcluded,
+Configuration config) {
+StringBuilder sb = new StringBuilder();
+if (!scalingEnabled) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+SCALING_ENABLED.key(),
+false));
+} else if (isExcluded) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+EXCLUDED_PERIODS.key(),
+config.get(EXCLUDED_PERIODS)));
+} else {
+sb.append(SCALING_SUMMARY_HEADER_SCALING_EXECUTION_ENABLED);
+}
Review Comment:
I think it would make the code much cleaner to simply submit a string to the
event handler instead of the 2 booleans.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434212997
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Sorry, I misunderstood the code, I am fine with this :)
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434211420
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
Ah sorry I did not realise that you also changed the event message for the
basic recommendations... my bad
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434209212
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
In the current logic we send 2 types of events on a parallelism change:
1. Scaling Recommendation (when disabled) -> `Recommended parallelism
change: {parallelisms}`
2. Scaling Event (when enabled) -> `Scaling Vertices: {parallelisms}`
The PR basically adds a 3rd type of event which looks like a recommendation
but it doesn't say recommendation it prints:
`Scaling execution disabled by config: configkey:false {parallelism:map}`
I don't think that this is fully consistent with the current logic. We
intended to keep the recommendations, but we changed the code so that it's not
a recommendation anymore but looks different.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434199585
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I don't feel strongly about including the cause. We can also just send out
scaling events only for actual scaling operations. What @flashJd did is
actually more in line with the current code which does include meta information
like "scalingEnabled" in the event message. If we decide to skip those, for
consistency, I would also remove the "scalingEnabled" flag and only ever emit
scaling events when scaling is enabled and not blocked.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1434066148
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
If scaling is blocked or disabled the event that we send out is a
recommendation. Wouldn't it make sense to have uniform events for
recommendation regardless of the cause? The user always knows why it's only a
recommendation, either because the autoscaler is disabled completely or if in a
blocking period
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433991220
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/event/AutoScalerEventHandler.java:
##
@@ -83,12 +92,27 @@ default void handleScalingEvent(
}
static String scalingReport(
-Map scalingSummaries, boolean
scalingEnabled) {
-StringBuilder sb =
-new StringBuilder(
-scalingEnabled
-? SCALING_SUMMARY_HEADER_SCALING_ENABLED
-: SCALING_SUMMARY_HEADER_SCALING_DISABLED);
+Map scalingSummaries,
+boolean scalingEnabled,
+boolean isExcluded,
+Configuration config) {
+StringBuilder sb = new StringBuilder();
+if (!scalingEnabled) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+SCALING_ENABLED.key(),
+false));
+} else if (isExcluded) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+EXCLUDED_PERIODS.key(),
+config.get(EXCLUDED_PERIODS)));
+} else {
+sb.append(SCALING_SUMMARY_HEADER_SCALING_EXECUTION_ENABLED);
+}
Review Comment:
I also didn't like this change at first, but I think the additional code
here is justified to provide better error reporting to the user.
As an alternative, we could replace the `scalingEnabled` and
`scalingBlocked` flags via a generic String which allows to pass in any
additional explanation by the caller.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433989601
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I was contemplating to comment on this as well, but after looking at the
code, there is some benefit to hinting whether scaling is disabled or blocked.
By treating blocked scaling as "disabled", we yield less information to the
user. I think it will be beneficial to learn WHY the scaling is disabled, i.e.
scaling disabled or scaling blocked.
>The event handler really has nothing to do with this feature (and many
other things that happen in the scaling executor)
Not sure about that. Event handlers are supposed to process events and in
the context of the autoscaler, a blocked scaling decision is a type of event we
may present to the user. We are already leaking whether scaling is enabled or
not to the event handler, why not also annotate why it is disabled?
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433973975
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
The event handler really has nothing to do with this feature (and many other
things that happen in the scaling executor)
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433973067
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I am not a big fan of scattering features across many interfaces without a
very good reason. It makes the event handler and the whole logic more complex
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433921836
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I've considered just print a log but finally chose adjust autoscaler event
handler a bit:
1. Print a log will cause many annoying duplicated logs if exludedPeriods
set is relatively wide
2. Now that we have the interval based event trigger method, why not use it
@mxm what do you think
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433757961
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,21 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+ return !scaleEnabled || isExcluded;
Review Comment:
I don't completely understand why we had to move this into a method. It
could simply be:
```
var scaleEnabled = conf.get(SCALING_ENABLED) &&
!CalendarUtils.inExcludedPeriods(conf, now);
autoScalerEventHandler.handleScalingEvent(
context,
scalingSummaries,
scaleEnabled,
conf.get(SCALING_EVENT_INTERVAL));
```
Then we don't need any changes to the autoscaler event handler, we could
simply log a line here that we are in an excluded period instead of changing
code in multiple places.
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/event/AutoScalerEventHandler.java:
##
@@ -83,12 +92,27 @@ default void handleScalingEvent(
}
static String scalingReport(
-Map scalingSummaries, boolean
scalingEnabled) {
-StringBuilder sb =
-new StringBuilder(
-scalingEnabled
-? SCALING_SUMMARY_HEADER_SCALING_ENABLED
-: SCALING_SUMMARY_HEADER_SCALING_DISABLED);
+Map scalingSummaries,
+boolean scalingEnabled,
+boolean isExcluded,
+Configuration config) {
+StringBuilder sb = new StringBuilder();
+if (!scalingEnabled) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+SCALING_ENABLED.key(),
+false));
+} else if (isExcluded) {
+sb.append(
+String.format(
+SCALING_SUMMARY_HEADER_SCALING_EXECUTION_DISABLED,
+EXCLUDED_PERIODS.key(),
+config.get(EXCLUDED_PERIODS)));
+} else {
+sb.append(SCALING_SUMMARY_HEADER_SCALING_EXECUTION_ENABLED);
+}
Review Comment:
as I wrote in the other comment, I prefer not to make any changes in the
event handler
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1433376746
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,24 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+if (!scaleEnabled || isExcluded) {
+return true;
+}
+return false;
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-186482 > @gyfora @mxm by the way, I'd like to discuss the logic of scaling effectiveness evaluation with you. > > 1. Now it's controlled by two config `scaling.effectiveness.detection.enabled` and `scaling.effectiveness.threshold` and we evaluate the effectiveness under the condition `last scaling is scale up` and only refer to the last scaling effectiveness. > 2. Image the following scenario: scale up double parallism first, then scale down to 0.8 parallism, then scale up double scale down 0.8, the effectiveness detection will be invalid in this scenario, even scale up is ineffecive, we'll continue scale up > 3. Maybe we can add a new config like `scaling.effectiveness.history.reference.num` and set a default value, then we can evaluate based on the last `scaling.effectiveness.history.reference.num` scale up summaries. > > Looking forward to your reply. What yo you describe is an edge case we hadn't considered. We were more concerned about a continuous increase in parallelism. If there is any scale down we are currently assuming that the algorithm is not completely broken. -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1432942758
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingExecutor.java:
##
@@ -264,4 +262,24 @@ private static Map
getVertexParallelismOverrides(
});
return overrides;
}
+
+private boolean blockScalingExecution(
+Context context,
+Map scalingSummaries,
+Configuration conf,
+Instant now) {
+var scaleEnabled = conf.get(SCALING_ENABLED);
+var isExcluded = CalendarUtils.inExcludedPeriods(conf, now);
+autoScalerEventHandler.handleScalingEvent(
+context,
+scalingSummaries,
+scaleEnabled,
+isExcluded,
+conf.get(SCALING_EVENT_INTERVAL));
+
+if (!scaleEnabled || isExcluded) {
+return true;
+}
+return false;
Review Comment:
```suggestion
return !scaleEnabled || isExcluded;
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1864238511 @gyfora @mxm by the way, I'd like to discuss the logic of scaling effectiveness evaluation with you. 1. Now it's controlled by two config `scaling.effectiveness.detection.enabled` and `scaling.effectiveness.threshold` and we evaluate the effectiveness under the condition `last scaling is scale up` and only refer to the last scaling effectiveness. 2. Image the following scenario: scale up double parallism first, then scale down to 0.8 parallism, then scale up double scale down 0.8, the effectiveness detection will be invalid in this scenario, even scale up is ineffecive, we'll continue scale up 3. Maybe we can add a new config like `scaling.effectiveness.history.reference.num` and set a default value, then we can evaluate based on the last `scaling.effectiveness.history.reference.num` scale up summaries. Looking forward to your reply. -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1432470030
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -147,7 +149,13 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}",
readable(windowFullTime));
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info(
+"Autoscaling on halt based on exclusion rule {}",
+conf.get(AutoScalerOptions.EXCLUDED_PERIODS));
+} else {
+collectedMetrics.setFullyCollected(true);
+}
Review Comment:
@gyfora @mxm you are right, it's better to place the excludedPeriods
blocking logic in ScalingExecutor. As the meaning of excludedPeriods is very
similar with config `scaling.enabled`, i combines the logic in
`ScalingExecutor`. Now excludedPeriods blocking works well with recommended
parallelism
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1431648293
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -147,7 +149,13 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}",
readable(windowFullTime));
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info(
+"Autoscaling on halt based on exclusion rule {}",
+conf.get(AutoScalerOptions.EXCLUDED_PERIODS));
+} else {
+collectedMetrics.setFullyCollected(true);
+}
Review Comment:
Although the current logic works fine for blocking the scaling during the
configured phases, I agree that the logic would be better suited for
ScalingExecutor. There are other features, e.g. recommended parallelism, which
will stop working if the metric collection phase hasn't completed. We only want
to block executing the scaling. All other logic should run normally.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
gyfora commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1431635885
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -147,7 +149,13 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}",
readable(windowFullTime));
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info(
+"Autoscaling on halt based on exclusion rule {}",
+conf.get(AutoScalerOptions.EXCLUDED_PERIODS));
+} else {
+collectedMetrics.setFullyCollected(true);
+}
Review Comment:
Enforcing this through collectedMetrics.setFullyCollected(true); seems a bit
hacky and susceptible to breaking if we treat the collected metrics differently.
Maybe we should move this check to the `ScalingExecutor`
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -147,7 +149,13 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}",
readable(windowFullTime));
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info(
+"Autoscaling on halt based on exclusion rule {}",
+conf.get(AutoScalerOptions.EXCLUDED_PERIODS));
+} else {
+collectedMetrics.setFullyCollected(true);
+}
Review Comment:
what do you think @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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1863023384 @gyfora Any more comments? -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1862843009 @mxm can you approval the workflow again, I've fixed the ci failure, thanks. -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1430882288
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -143,7 +145,11 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}", windowFullTime);
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info("autoscaling now in excluded period");
Review Comment:
Agreed. Adjustment has been made
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#issuecomment-1859986239 Thanks @flashJd! Changes look great. -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1429838068
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/ScalingMetricCollector.java:
##
@@ -143,7 +145,11 @@ public CollectedMetricHistory updateMetrics(
if (now.isBefore(windowFullTime)) {
LOG.info("Metric window not full until {}", windowFullTime);
} else {
-collectedMetrics.setFullyCollected(true);
+if (isExcluded) {
+LOG.info("autoscaling now in excluded period");
Review Comment:
```suggestion
LOG.info("Autoscaling on halt based on exclusion rule {}",
conf.get(AutoScalerOptions. EXCLUDED_PERIODS));
```
I think this could be helpful for debugging.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426756716
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/AutoScalerUtils.java:
##
@@ -94,4 +99,106 @@ public static boolean excludeVerticesFromScaling(
conf.set(AutoScalerOptions.VERTEX_EXCLUDE_IDS, new
ArrayList<>(excludedIds));
return anyAdded;
}
+
+/** Quartz doesn't have the invertTimeRange flag so rewrite this method. */
+static boolean isTimeIncluded(CronCalendar cron, long timeInMillis) {
+if (cron.getBaseCalendar() != null
+&& !cron.getBaseCalendar().isTimeIncluded(timeInMillis)) {
+return false;
+} else {
+return cron.getCronExpression().isSatisfiedBy(new
Date(timeInMillis));
+}
+}
+
+static Optional interpretAsDaily(String subExpression) {
+String[] splits = subExpression.split("-");
+if (splits.length != 2) {
+return Optional.empty();
+}
+try {
+DailyCalendar daily = new DailyCalendar(splits[0], splits[1]);
+daily.setInvertTimeRange(true);
+return Optional.of(daily);
+} catch (Exception e) {
+return Optional.empty();
+}
+}
+
+static Optional interpretAsCron(String subExpression) {
+try {
+return Optional.of(new CronCalendar(subExpression));
+} catch (Exception e) {
+return Optional.empty();
Review Comment:
I think we should log this exception. Otherwise it is going to be hard to
figure out what is wrong with the cron string. This also applies to all other
methods in this class which have this pattern.
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1429453681
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/AutoScalerUtils.java:
##
@@ -94,4 +99,106 @@ public static boolean excludeVerticesFromScaling(
conf.set(AutoScalerOptions.VERTEX_EXCLUDE_IDS, new
ArrayList<>(excludedIds));
return anyAdded;
}
+
+/** Quartz doesn't have the invertTimeRange flag so rewrite this method. */
+static boolean isTimeIncluded(CronCalendar cron, long timeInMillis) {
+if (cron.getBaseCalendar() != null
+&& !cron.getBaseCalendar().isTimeIncluded(timeInMillis)) {
+return false;
+} else {
+return cron.getCronExpression().isSatisfiedBy(new
Date(timeInMillis));
+}
+}
+
+static Optional interpretAsDaily(String subExpression) {
+String[] splits = subExpression.split("-");
+if (splits.length != 2) {
+return Optional.empty();
+}
+try {
+DailyCalendar daily = new DailyCalendar(splits[0], splits[1]);
+daily.setInvertTimeRange(true);
+return Optional.of(daily);
+} catch (Exception e) {
+return Optional.empty();
+}
+}
+
+static Optional interpretAsCron(String subExpression) {
+try {
+return Optional.of(new CronCalendar(subExpression));
+} catch (Exception e) {
+return Optional.empty();
Review Comment:
Incorrect expression config will first be validated in `DefaultValidator`
and report exception there, I've added extra tests in `DefaultValidatorTest`
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1429452589
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/AutoScalerUtils.java:
##
@@ -94,4 +99,108 @@ public static boolean excludeVerticesFromScaling(
conf.set(AutoScalerOptions.VERTEX_EXCLUDE_IDS, new
ArrayList<>(excludedIds));
return anyAdded;
}
+
+/** Quartz doesn't have the invertTimeRange flag so rewrite this method. */
+static boolean isTimeIncluded(CronCalendar cron, long timeInMillis) {
Review Comment:
Removed to `CalendarUtils` and corresponding tests removed to
`CalendarUtilsTest`
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
mxm commented on code in PR #728:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1426756716
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/AutoScalerUtils.java:
##
@@ -94,4 +99,106 @@ public static boolean excludeVerticesFromScaling(
conf.set(AutoScalerOptions.VERTEX_EXCLUDE_IDS, new
ArrayList<>(excludedIds));
return anyAdded;
}
+
+/** Quartz doesn't have the invertTimeRange flag so rewrite this method. */
+static boolean isTimeIncluded(CronCalendar cron, long timeInMillis) {
+if (cron.getBaseCalendar() != null
+&& !cron.getBaseCalendar().isTimeIncluded(timeInMillis)) {
+return false;
+} else {
+return cron.getCronExpression().isSatisfiedBy(new
Date(timeInMillis));
+}
+}
+
+static Optional interpretAsDaily(String subExpression) {
+String[] splits = subExpression.split("-");
+if (splits.length != 2) {
+return Optional.empty();
+}
+try {
+DailyCalendar daily = new DailyCalendar(splits[0], splits[1]);
+daily.setInvertTimeRange(true);
+return Optional.of(daily);
+} catch (Exception e) {
+return Optional.empty();
+}
+}
+
+static Optional interpretAsCron(String subExpression) {
+try {
+return Optional.of(new CronCalendar(subExpression));
+} catch (Exception e) {
+return Optional.empty();
Review Comment:
I think we should log this exception. Otherwise it is going to be hard to
figure out what is wrong with the cron string. This all other methods in this
class which have this pattern.
##
flink-autoscaler/src/main/java/org/apache/flink/autoscaler/utils/AutoScalerUtils.java:
##
@@ -94,4 +99,108 @@ public static boolean excludeVerticesFromScaling(
conf.set(AutoScalerOptions.VERTEX_EXCLUDE_IDS, new
ArrayList<>(excludedIds));
return anyAdded;
}
+
+/** Quartz doesn't have the invertTimeRange flag so rewrite this method. */
+static boolean isTimeIncluded(CronCalendar cron, long timeInMillis) {
Review Comment:
Can we move these methods to a new class, e.g. `DateUtils` or `CronUtils`?
--
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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
flashJd commented on code in PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1427581625 ## flink-autoscaler/pom.xml: ## @@ -66,6 +66,12 @@ under the License. jackson-databind + +org.quartz-scheduler +quartz +2.3.2 Review Comment: sure, already extracted -- 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 a new config to forbid autoscale execution in the configured excluded periods [flink-kubernetes-operator]
1996fanrui commented on code in PR #728: URL: https://github.com/apache/flink-kubernetes-operator/pull/728#discussion_r1427486350 ## flink-autoscaler/pom.xml: ## @@ -66,6 +66,12 @@ under the License. jackson-databind + +org.quartz-scheduler +quartz +2.3.2 Review Comment: Would you mind extract a property for this version? Such as : `quartz.version`. -- 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
