[
https://issues.apache.org/jira/browse/GOBBLIN-1880?focusedWorklogId=878227&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-878227
]
ASF GitHub Bot logged work on GOBBLIN-1880:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 24/Aug/23 16:56
Start Date: 24/Aug/23 16:56
Worklog Time Spent: 10m
Work Description: 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!
Issue Time Tracking
-------------------
Worklog Id: (was: 878227)
Time Spent: 1h 20m (was: 1h 10m)
> Handle Invalid Cron Schedules for Gobblin Scheduler
> ---------------------------------------------------
>
> Key: GOBBLIN-1880
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1880
> Project: Apache Gobblin
> Issue Type: New Feature
> Reporter: Meeth Gala
> Priority: Major
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
