-
Notifications
You must be signed in to change notification settings - Fork 3
Add Agenda class for task scattering over time periods #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Introduces the Agenda class that collects tasks and distributes them evenly over a time period. Key features: - Scatter tasks over a timeframe with even distribution - Optional jitter to prevent thundering herd - Single task positioned at midpoint of window - Required 'over' parameter for explicit timing Still needs true atomicity implementation using Redis pipelines. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The scatter method now provides true atomic scheduling - either all tasks are scheduled successfully or none are. This is achieved by: 1. Validating all tasks upfront before scheduling any 2. Building Execution objects with validation 3. Rolling back (cancelling) any scheduled tasks if a failure occurs The atomicity test now properly demonstrates: - Failure when unregistered tasks are included (no tasks scheduled) - Successful scheduling when all tasks are valid - Rollback behavior when scheduling fails partway through This ensures predictable behavior for batch task scheduling operations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Added tests to cover the remaining uncovered lines: - Auto-registration of unregistered task functions (line 172) - Exception handling during rollback when cancel fails (lines 205-206) The agenda module now has full test coverage demonstrating all code paths work correctly, including edge cases. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Removes the complex compensation-based rollback in favor of simple, predictable behavior: - No rollback attempts when scheduling fails - Clear documentation about partial scheduling - Honest test expectations about failure behavior - Removed misleading atomicity claims Also: - Removed task scattering from main README (too detailed) - Added comprehensive Agenda documentation to advanced-patterns.md - Maintained 100% test coverage with simplified implementation The behavior is now: if scheduling fails partway through, earlier tasks remain scheduled and later tasks are not scheduled. This is simple, predictable, and honest about the limitations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Claude finished @chrisguidry's task —— View job Code Review for PR #159: Add Agenda class for task scatteringTodo List
SummaryThis PR successfully implements a well-designed ✅ Strengths
🎯 Code Quality Observations
💡 Minor Suggestions for Future Improvements
🔒 Security ConsiderationsNo security concerns identified. The implementation properly:
⚡ Performance ConsiderationsThe implementation is efficient for typical use cases:
🐛 No Bugs FoundThe implementation appears bug-free with robust error handling and edge case coverage. ✅ RecommendationThis PR is ready to merge. The |
|
📚 Documentation has been built for this PR! You can download the documentation directly here: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 31 33 +2
Lines 4536 4789 +253
Branches 247 267 +20
==========================================
+ Hits 4536 4789 +253
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The example was using Worker without the async context manager, causing AttributeError on _execution_counts. Also reduced timing windows to make the example complete within 2 minutes instead of 3+ minutes. Fixed: - Worker now properly used with async context manager - Reduced scatter windows from 30s/20s to 10s/10s - Reduced future window from 2min to 30s - Reduced worker timeout from 3min to 1.5min - Reduced task sleep times from 0.5s/0.2s to 0.1s/0.1s 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@claude let's implement handling for your code review suggestion:
|
|
Claude finished @chrisguidry's task —— View job Task: Implement safety check for jitter in Agenda.scatter()Todo List
SummarySuccessfully implemented the safety check to prevent jittered times from going before the start time. The changes include:
The fix ensures that no matter how large the jitter value is, tasks will never be scheduled before the specified start time, preventing potential negative timestamp issues. |
When using large jitter values, the random offset could potentially push scheduled times before the start time. Added a safety check to clamp jittered times to never be earlier than the start time. Added test case to verify this edge case is handled correctly. Co-authored-by: Chris Guidry <[email protected]>
|
@claude let's add that validation for |
|
Claude finished @chrisguidry's task —— View job Task: Add validation for
|
- Ensure 'over' parameter is positive (> 0 seconds) - Add test cases for zero and negative durations - Update docstring to document the ValueError Co-authored-by: Chris Guidry <[email protected]>
Summary
Adds an
Agendaclass that collects tasks and scatters them evenly over time periods to avoid overwhelming systems during find-and-flood workloads.The implementation provides a clean API for batch scheduling:
agenda.add(task)(args)to collect tasksagenda.scatter(docket, over=timedelta(minutes=50))to distribute themKey design decisions made during implementation:
overparameter is required for explicit timing controlCloses #131
🤖 Generated with Claude Code