Make scheduler use lastExecutedAt instead of next planned executeAt #73
Conversation
evlekht
force-pushed
the
evlekht/cash-in-schedule
branch
from
200cd6f to
cc10514
Compare
evlekht
changed the title
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the job scheduling system within the application. The primary alteration is the replacement of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
pkg/scheduler/scheduler.go (2)
120-123: Consider adding a log statement on context cancellationIn the
selectstatement, the case for<-timersCtx.Done()>is empty. Adding a log statement here can help with debugging and monitoring by indicating that the timer has been cancelled.
174-183: Consider initializingLastExecutedAtto current time for new jobsWhen scheduling a new job (
job == nil),LastExecutedAtis set to the zero time (time.Time{}). This causes the job to execute immediately upon scheduler start. If it's preferable for new jobs to have their first execution after a full period, consider initializingLastExecutedAttos.clock.Now(). This ensures the first execution occurs after the specified period.pkg/database/sqlite/storage.go (1)
38-40: Consider makinglogMigrationsconfigurableCurrently,
logMigrationsis hardcoded tofalsein the call tos.migrate. Consider adding a configuration option or command-line flag to enable migration logging when needed.pkg/scheduler/scheduler_test.go (1)
Line range hint
268-273: Consider adding test cases for edge casesWhile the current test coverage is good, consider adding test cases for:
- Zero Period validation
- Very large Period values
- Negative LastExecutedAt values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
migrations/scheduler/2_last_executed_at.down.sql(1 hunks)migrations/scheduler/2_last_executed_at.up.sql(1 hunks)pkg/database/sqlite/storage.go(3 hunks)pkg/scheduler/job.go(1 hunks)pkg/scheduler/scheduler.go(5 hunks)pkg/scheduler/scheduler_test.go(7 hunks)pkg/scheduler/storage/sqlite/jobs.go(3 hunks)
🔇 Additional comments (9)
pkg/scheduler/scheduler.go (1)
158-159: Update the comment to reflect current behavior
The comment states: "If there is already scheduled job with the same name, then its period is updated." Please verify that this accurately describes the current behavior and update it if necessary.
pkg/scheduler/job.go (1)
6-8: Updated Job struct fields look good
The changes to the Job struct, replacing ExecuteAt with LastExecutedAt, align with the new scheduling logic and are correctly implemented.
pkg/database/sqlite/storage.go (1)
Line range hint 58-90: Conditional migration logging implementation looks good
The addition of migrationLogger and the logMigrations parameter provides flexibility in logging migration details. The implementation correctly adheres to the migrate.Logger interface.
pkg/scheduler/storage/sqlite/jobs.go (3)
19-21: LGTM: Job struct updated to use LastExecutedAt
The struct field change from ExecuteAt to LastExecutedAt is well-implemented with the correct DB tag.
106-116: LGTM: SQL query updated for LastExecutedAt
The SQL upsert query has been properly updated to use last_executed_at in both INSERT and UPDATE clauses.
138-140: LGTM: Model conversion functions updated correctly
The model conversion functions modelFromJob and jobFromModel have been properly updated to handle the LastExecutedAt field while maintaining correct timestamp conversions.
Also applies to: 146-148
pkg/scheduler/scheduler_test.go (3)
31-65: LGTM: Test cases updated with comprehensive coverage
The test setup has been enhanced with:
- Updated job definitions using
LastExecutedAt - Added
freshJobtest case for never-executed jobs - Well-defined periods and execution times for each test case
105-128: LGTM: Job execution timing logic updated correctly
The test execution logic has been properly updated to:
- Track last execution times
- Calculate next execution based on LastExecutedAt + Period
- Handle both initial and subsequent executions
Line range hint 278-312: LGTM: Schedule test cases updated appropriately
The Schedule test has been properly updated with:
- Simplified storage mock function signature
- Correct handling of LastExecutedAt in test cases
- Comprehensive coverage for both new and existing jobs
Summary by CodeRabbit
ExecuteAtfield withLastExecutedAt, improving tracking of job execution times.LastExecutedAtfield.