[DSIP-18454][Scheduler] Add schedule missed fire policy - #18464
[DSIP-18454][Scheduler] Add schedule missed fire policy#18464liang-wenjie wants to merge 8 commits into
Conversation
| private Date endTime; | ||
| private String crontab; | ||
| private String timezoneId; | ||
| private ScheduleMissedFirePolicy missedFirePolicy = ScheduleMissedFirePolicy.FIRE_ONCE_NOW; |
There was a problem hiding this comment.
Right now, the default misfire policy is FIRE_ALL_MISSED?
There was a problem hiding this comment.
The previous implementation did not set an explicit policy, so Quartz used SMART_POLICY. In Quartz 2.3.2, CronTriggerImpl.updateAfterMisfire() explicitly translates SMART_POLICY to MISFIRE_INSTRUCTION_FIRE_ONCE_NOW. Therefore, I kept FIRE_ONCE_NOW as the default to preserve the current behavior rather than changing existing schedules to FIRE_ALL_MISSED.
There was a problem hiding this comment.
Which version are you using? ds use withMisfireHandlingInstructionIgnoreMisfires to set the policy.
There was a problem hiding this comment.
You are right. DolphinScheduler uses Quartz 2.3.2, and the current dev implementation explicitly calls withMisfireHandlingInstructionIgnoreMisfires(), which maps to MISFIRE_INSTRUCTION_IGNORE_MISFIRE_POLICY. I had incorrectly reasoned from Quartz SMART_POLICY instead of checking the existing builder call. Updated in 2868f09: API/database/UI defaults and null fallback now use FIRE_ALL_MISSED, and the default factory test verifies the existing IgnoreMisfires behavior.
|
|
||
| import org.quartz.CronScheduleBuilder; | ||
|
|
||
| final class QuartzScheduleMissedFirePolicyApplier { |
There was a problem hiding this comment.
Don't add this util, it's better to create CronScheduleBuilderFactory and add three different implementation.
There was a problem hiding this comment.
Updated in c73327e. I replaced the util with CronScheduleBuilderFactory and added three separate implementations: SkipMissedCronScheduleBuilderFactory, FireOnceNowCronScheduleBuilderFactory, and FireAllMissedCronScheduleBuilderFactory. The focused test now verifies both factory selection and the Quartz misfire instruction for each policy.
| start_time: '开始时间', | ||
| end_time: '结束时间', | ||
| crontab: 'Crontab', | ||
| missed_fire_policy: '错过触发策略', |
There was a problem hiding this comment.
| missed_fire_policy: '错过触发策略', | |
| missed_fire_policy: '定时错过策略', |
There was a problem hiding this comment.
Updated to 定时错过策略 in c73327e. Thanks for the suggestion.
|
The latest commit 2868f09 addresses the review feedback and the previous CI issues. The Backend, Frontend, Test, API-Test, E2E, Docs, and CodeQL workflows currently show |
ruanwenjun
left a comment
There was a problem hiding this comment.
Should also update incompatible.md
| private ScheduleMissedFirePolicy defaultMissedFirePolicy(ScheduleMissedFirePolicy missedFirePolicy) { | ||
| return missedFirePolicy == null ? ScheduleMissedFirePolicy.FIRE_ALL_MISSED : missedFirePolicy; | ||
| } |
There was a problem hiding this comment.
Is this needed? Your already set a initialize value at scheduleParam.
There was a problem hiding this comment.
Removed the redundant fallback helper. SchedulerServiceImpl now uses ScheduleParam#getMissedFirePolicy directly, since ScheduleParam already initializes the default value.
| public CronScheduleBuilder createCronScheduleBuilder(String cronExpression) { | ||
| return CronScheduleBuilder.cronSchedule(cronExpression) | ||
| .withMisfireHandlingInstructionFireAndProceed(); | ||
| } |
There was a problem hiding this comment.
| public CronScheduleBuilder createCronScheduleBuilder(String cronExpression) { | |
| return CronScheduleBuilder.cronSchedule(cronExpression) | |
| .withMisfireHandlingInstructionFireAndProceed(); | |
| } | |
| public CronScheduleBuilder createCronScheduleBuilder(Schedule schedule) { | |
| return CronScheduleBuilder.cronSchedule(cronExpression) | |
| .withMisfireHandlingInstructionFireAndProceed() | |
| .inTimeZone(DateUtils.getTimezone(schedule.getTimezoneId())); | |
| } |
There was a problem hiding this comment.
Updated CronScheduleBuilderFactory to accept the complete Schedule. All three implementations now create the cron schedule from Schedule#getCrontab and apply the schedule timezone internally. The factory tests also verify the configured timezone for every policy.
|
Addressed the latest review feedback in 060743f:
Local verification:
The previous dead-link failure is unrelated to this PR content: lychee v0.24.0 rejects the existing repository setting include_fragments = false (it expects a string or table). The milestone-label check still requires a maintainer-added milestone/type label. |
|
Thank you for the approval. I checked the remaining failed checks on commit 060743f:
I do not have repository admin permission to rerun these jobs. Could a maintainer please rerun the failed Test and Mergeable checks (and handle/rerun dead-link as appropriate)? The remaining Backend/API-Test/E2E/CodeQL workflows are still in progress. |
SbloodyS
left a comment
There was a problem hiding this comment.
The selected missed-fire policy is never submitted and updates reset it
The new selector writes its value to timingForm.missedFirePolicy, but use-modal.ts#getTimingData only serializes startTime, endTime, crontab, and timezoneId into the schedule request.
Consequently:
- Creating a schedule with
SKIP_MISSEDorFIRE_ONCE_NOWalways stores the defaultFIRE_ALL_MISSED; the selector currently has no effect. - Updating a schedule with a non-default policy omits the field, after which
ScheduleParamsuppliesFIRE_ALL_MISSEDandSchedulerServiceImpl#updateScheduleoverwrites the existing policy.
Please include missedFirePolicy in the schedule JSON constructed by getTimingData.
For backward compatibility, the backend should also distinguish creation defaults from an omitted update field: an omitted field during creation may use FIRE_ALL_MISSED, while an omitted field during update should preserve the currently stored policy. Please add create/update regression tests covering all three policies and an update request from a legacy client that omits the field.
…y on update
Address review feedback (SbloodyS):
- Frontend: include missedFirePolicy in the schedule create/update payload
so the selected policy is actually persisted to the backend.
- Backend: distinguish an omitted JSON field from an explicit value. A new
missedFirePolicySet marker tracks field presence, because Jackson cannot
tell omission from an explicit null.
- create: omitted or explicit null falls back to FIRE_ALL_MISSED
- update: when the client omits the field (e.g. an older client), the
existing stored policy is preserved instead of being overwritten
- Add unit tests covering create/update semantics and JSON presence detection.
Co-Authored-By: WorkBuddy <workbuddy@tencent.com>
|
Thanks for the review, @SbloodyS. I've addressed both points:
Let me know if you'd prefer a different default or behavior. |
|
You should check failed tests. @liang-wenjie |
…re-policy-v2' into 2dev/feat/add-schedule-missed-fire-policy-v2
|
Thanks, @SbloodyS. I checked and fixed the failed schedule API tests in The relevant checks are now green:
The only remaining failed check is Could you please re-review the missed-fire-policy changes when convenient? The requested frontend serialization, backward-compatible update behavior, and create/update regression coverage are all included in the current branch. Thank you. |
Was this PR generated or assisted by AI?
YES. AI assisted with reviewing the previous implementation, refining domain naming and compatibility behavior, adding focused tests, and preparing this pull request. The changes were reviewed and validated by the contributor.
Purpose of the pull request
This pull request adds a schedule-level missed fire policy for Cron schedules as an independent part of DSIP #18454.
It is split from the closed PR #18458 so that missed-fire handling can be reviewed separately from the fixed-interval trigger design. Fixed-interval scheduling is intentionally out of scope for this pull request.
The policy uses scheduler-domain terminology instead of Quartz-specific names. DolphinScheduler currently uses Quartz 2.3.2 and explicitly calls
withMisfireHandlingInstructionIgnoreMisfires()when building Cron triggers, so the default isFIRE_ALL_MISSEDto preserve the existing behavior.Related to #18454.
Supersedes the missed-fire policy portion of #18458.
Brief change log
ScheduleMissedFirePolicywithSKIP_MISSED,FIRE_ONCE_NOW, andFIRE_ALL_MISSED.missedFirePolicyin schedule API models and themissed_fire_policydatabase column.CronScheduleBuilderFactorywith separate implementations for all three policies.FIRE_ALL_MISSEDto preserve the currentIgnoreMisfiresbehavior.Verify this pull request
This change added tests and can be verified as follows:
CronScheduleBuilderFactoryTestcovering all policies and the default behavior.vue-tsc --noEmitpassed.javac; they are expected to run in CI.Pull Request Notice
Pull Request Notice
This pull request does not introduce an incompatible change. Existing schedules and requests that omit the new field retain the current Quartz
IgnoreMisfiresbehavior through theFIRE_ALL_MISSEDdefault.