Add failure policy in dapr schedular#69
Conversation
Reviewer's GuideIntroduces a configurable job failure policy for Dapr-scheduled background jobs, threading a new JobScheduleFailurePolicy abstraction through the background job APIs into the Dapr scheduler where it is mapped to Dapr’s IJobFailurePolicyOptions, while doing minor cleanup to scheduling calls and logging. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a ChangesBackground Job Scheduling Failure Policy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When updating jobs (e.g., in
UpdateScheduleAsyncandBackgroundJobService.UpdateAsync), the existing failure policy is not preserved or reapplied, so rescheduling a job will silently drop any configured policy; consider retrieving and reusing the existing policy to keep behavior consistent. MapFailurePolicysilently returnsnullfor unsupported or misconfiguredJobScheduleFailurePolicyvalues (e.g.,PolicyType.ConstantwithInterval == null), which can hide configuration errors; consider throwing or logging in these cases to surface misconfigurations early.- The new
failurePolicyOptionsparameter onIJobScheduler.ScheduleAsyncchanges the interface signature and will require all implementers to update their implementations; if this is intended, ensure downstream schedulers are updated, or consider a non-breaking extension mechanism if external implementations are expected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When updating jobs (e.g., in `UpdateScheduleAsync` and `BackgroundJobService.UpdateAsync`), the existing failure policy is not preserved or reapplied, so rescheduling a job will silently drop any configured policy; consider retrieving and reusing the existing policy to keep behavior consistent.
- `MapFailurePolicy` silently returns `null` for unsupported or misconfigured `JobScheduleFailurePolicy` values (e.g., `PolicyType.Constant` with `Interval == null`), which can hide configuration errors; consider throwing or logging in these cases to surface misconfigurations early.
- The new `failurePolicyOptions` parameter on `IJobScheduler.ScheduleAsync` changes the interface signature and will require all implementers to update their implementations; if this is intended, ensure downstream schedulers are updated, or consider a non-breaking extension mechanism if external implementations are expected.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a JobScheduleFailurePolicy to the background job system, enabling configurable retry strategies such as 'Drop' and 'Constant'. While the core logic for passing these policies to the scheduler is implemented, several issues were identified: the failure policy is not persisted in the BackgroundJobInfo entity, causing it to be lost during job reconstruction or updates. Furthermore, the policy is missing from update calls in both BackgroundJobService and DaprJobScheduler. Other feedback includes a recommendation to remove rather than comment out a package reference and to eliminate a redundant delete operation in the Dapr scheduler.
| uow.OnCompleted(async _ => | ||
| { | ||
| await jobScheduler.ScheduleAsync(handlerName, jobName, schedule, payloadBytes, cancellationToken); | ||
| await jobScheduler.ScheduleAsync(handlerName, jobName, schedule, payloadBytes, failurePolicyOptions, cancellationToken); |
There was a problem hiding this comment.
While the failure policy is correctly passed to the scheduler here, it is not being persisted in the BackgroundJobInfo entity (the jobInfo object created on line 97). Since this service integrates job persistence with scheduling, the failure policy will be lost if the job needs to be reconstructed from the database or updated later. Consider adding a field to BackgroundJobInfo or storing the policy within ExtraProperties.
| var payloadBytes = eventSerializer.Serialize(envelope); | ||
| await jobScheduler.ScheduleAsync(jobInfo.HandlerName, jobInfo.JobName, newSchedule, payloadBytes, | ||
| cancellationToken); | ||
| cancellationToken: cancellationToken); |
There was a problem hiding this comment.
The failurePolicyOptions parameter is missing from this ScheduleAsync call. When a job's schedule is updated, the original failure policy should be retrieved from the job store and passed here to ensure it is preserved. Currently, updating a job effectively resets its failure policy to null.
| var jobInfo = await daprJobsClient.GetJobAsync(jobName, cancellationToken); | ||
| await daprJobsClient.DeleteJobAsync(jobName, cancellationToken); | ||
| await ScheduleAsync(handlerName, jobName, newSchedule, jobInfo.Payload, cancellationToken); | ||
| await ScheduleAsync(handlerName, jobName, newSchedule, jobInfo.Payload, cancellationToken: cancellationToken); |
There was a problem hiding this comment.
This call to ScheduleAsync does not pass the failure policy, causing it to be lost during schedule updates. You should extract the existing failure policy from the jobInfo object (returned by Dapr on line 95) and pass it to the ScheduleAsync method. Additionally, since ScheduleAsync uses overwrite: true in its implementation, the explicit call to DeleteJobAsync on line 96 is redundant and creates a non-atomic operation that could lead to job loss if the subsequent scheduling fails.
| <PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" /> | ||
| <PackageReference Include="OpenTelemetry.Instrumentation.Http" /> | ||
| <PackageReference Include="OpenTelemetry.Exporter.Zipkin" /> | ||
| <!-- <PackageReference Include="OpenTelemetry.Exporter.Zipkin" />--> |
There was a problem hiding this comment.
Commenting out package references is generally discouraged as it leaves dead code in the project file. If the Zipkin exporter is no longer needed, it should be removed entirely. If it is intended to be optional or used only in specific environments, consider using conditional property groups or documenting the reason for keeping it commented out.
|
1 participant
Summary by Sourcery
Add configurable failure policies to background job scheduling and propagate them through the background job service and scheduler interfaces.
New Features:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Chores