meethngala commented on PR #3742:
URL: https://github.com/apache/gobblin/pull/3742#issuecomment-1692078956
> What would the user experience be if they added an invalid Cron? I think
we want to ensure that the user receives an errorcode 400 as well as some
information as to what the issue is with their cron. Potentially see
`flowSpec.addCompilationError` for how this is handled. Alternatively, consider
adding this check in `BaseFlowToJobSpecCompiler`, which is extended by other
compilers in GaaS, because it already sort of tries to handle this, but is
missing your additional checks to ensure that the schedule is actually valid.
>
> ```
> if (flowSpecProperties.containsKey(ConfigurationKeys.JOB_SCHEDULE_KEY)
&& StringUtils.isNotBlank(
>
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY))) {
> try {
> new
CronExpression(flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY));
> } catch (Exception e) {
> log.error("invalid cron schedule: {}",
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY), e);
> flowSpec.getCompilationErrors().add(new
FlowSpec.CompilationError(0, "invalid cron schedule: " +
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY) +
e.getMessage()));
> return null;
> }
> ```
>
>
https://github.com/apache/gobblin/blob/69d7e0fa4df2499934462854514ddc9ddbfe7dc7/gobblin-service/src/main/java/org/apache/gobblin/service/modules/flow/BaseFlowToJobSpecCompiler.java#L185
As part of this PR, I mainly want to cover scenarios wherein we try to
schedule an outdated cron which now expired. We are defaulting it to pass true
and running into NPE while scheduling the FlowSpec. As part
> What would the user experience be if they added an invalid Cron? I think
we want to ensure that the user receives an errorcode 400 as well as some
information as to what the issue is with their cron. Potentially see
`flowSpec.addCompilationError` for how this is handled. Alternatively, consider
adding this check in `BaseFlowToJobSpecCompiler`, which is extended by other
compilers in GaaS, because it already sort of tries to handle this, but is
missing your additional checks to ensure that the schedule is actually valid.
>
> ```
> if (flowSpecProperties.containsKey(ConfigurationKeys.JOB_SCHEDULE_KEY)
&& StringUtils.isNotBlank(
>
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY))) {
> try {
> new
CronExpression(flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY));
> } catch (Exception e) {
> log.error("invalid cron schedule: {}",
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY), e);
> flowSpec.getCompilationErrors().add(new
FlowSpec.CompilationError(0, "invalid cron schedule: " +
flowSpecProperties.getProperty(ConfigurationKeys.JOB_SCHEDULE_KEY) +
e.getMessage()));
> return null;
> }
> ```
>
>
https://github.com/apache/gobblin/blob/69d7e0fa4df2499934462854514ddc9ddbfe7dc7/gobblin-service/src/main/java/org/apache/gobblin/service/modules/flow/BaseFlowToJobSpecCompiler.java#L185
As part of this PR, I am trying to capture cases wherein we try to schedule
an outdated cron which has expired and then run into NPE when scheduling the
FlowSpec. Previously, we were defaulting it to pass true for any cron
expression that wouldn't pass the number of days constraint within the range.
Now, as per your above ask, I don't see this to be a normal user behavior
wherein the user would pass in a valid cron schedule which is in the past e.g.
`0 0 0 ? * 6L 2020`. But in case we want to fail fast and handle such scenarios
too... I would create a separate PR for that and limit this PR to handle cases
from the scheduler standpoint alone. Let me know if that makes sense!
--
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]
