phet commented on code in PR #3742:
URL: https://github.com/apache/gobblin/pull/3742#discussion_r1296246621
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -301,15 +305,17 @@ public static boolean isWithinRange(String
cronExpression, int maxNumDaysToSched
if (nextValidTimeAfter == null) {
log.warn("Calculation issue for next valid time for expression: {}.
Will default to true for within range",
cronExpression);
- return true;
+ return false;
Review Comment:
please characterize why the change from current/prior behavior--was it a
faulty presumption?
a code comment would go a long way in communicating the reasoning.
aso, log msg is now out-of-sync w/ behavior
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -291,6 +291,10 @@ private boolean addSpecHelperMethod(Spec spec) {
*/
@VisibleForTesting
public static boolean isWithinRange(String cronExpression, int
maxNumDaysToScheduleWithin) {
+ if (cronExpression == null || cronExpression.trim().isEmpty()) {
+ // If the cron expression is empty or null, return true to capture adhoc
flows
+ return true;
+ }
Review Comment:
at the core, I understand why we don't continue w/ `cronExpression ==
null`... but is this really happening? more canonical would be to have callers
avoid calling this w/ `null`. then add a `Preconditions` here that croaks if
`null` is passed in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
