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