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<DailyCalendar> 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<CronCalendar> 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

Reply via email to