fix: Add task leases to prevent a race scheduling the same task multiple times (fixes #105). - #98
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant SS as Scheduler Server/Main
participant FP as FifoPolicy
participant MS as MetadataStorage
participant DB as MySQLStorage
SS->>FP: Instantiate FifoPolicy(scheduler_id, metadata_store, data_store, conn)
FP->>MS: get_ready_tasks(conn, scheduler_id, tasks)
MS->>DB: Execute SQL queries (task leasing, cleanup)
DB-->>MS: Return ready tasks
MS-->>FP: Return tasks
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
There was a problem hiding this comment.
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- tools/scripts/storage/init_db.sql: Language not supported
Comments suppressed due to low confidence (1)
src/spider/storage/mysql/MySqlStorage.cpp:1229
- The lease expiration time of 10ms may be too short for production environments, risking premature lease removal. Consider reviewing and increasing the timeout duration if necessary.
constexpr int cLeaseExpireTime = 1000 * 10; // 10 ms
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/scheduler/test-SchedulerPolicy.cpp (1)
181-181: Helpful clarifying comment addedThe addition of this comment helps clarify the structure of the test by explicitly marking the section that adds the task.
tools/scripts/storage/init_db.sql (1)
109-117: Consistent implementation of scheduler_leases table in SQL scriptThe table creation statement matches the definition in
mysql_stmt.hpp, ensuring consistency across the codebase. The table design properly supports the task lease mechanism with:
- Proper foreign key constraints
- Timestamp for lease expiration tracking
- Index on scheduler_id for query optimization
Consider adding a brief comment above the table creation statement to explain its purpose for future developers.
src/spider/storage/mysql/MySqlStorage.hpp (1)
82-86: Updated method signature to support task leasingThe addition of the
scheduler_idparameter to theget_ready_tasksmethod is a key change that enables the implementation of the task lease mechanism. This change aligns with the modifications inFifoPolicy.cppand the new database schema.Consider adding a comment in the method declaration to explain the purpose of the new parameter and how it relates to the task leasing system.
auto get_ready_tasks( StorageConnection& conn, boost::uuids::uuid scheduler_id, std::vector<ScheduleTaskMetadata>* tasks -) -> StorageErr override; +) -> StorageErr override; // scheduler_id is used to track task leases and prevent duplicate scheduling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/spider/scheduler/FifoPolicy.cpp(2 hunks)src/spider/scheduler/FifoPolicy.hpp(2 hunks)src/spider/scheduler/scheduler.cpp(2 hunks)src/spider/storage/MetadataStorage.hpp(2 hunks)src/spider/storage/mysql/MySqlStorage.cpp(8 hunks)src/spider/storage/mysql/MySqlStorage.hpp(1 hunks)src/spider/storage/mysql/mysql_stmt.hpp(3 hunks)tests/scheduler/test-SchedulerPolicy.cpp(6 hunks)tests/scheduler/test-SchedulerServer.cpp(1 hunks)tests/storage/test-MetadataStorage.cpp(1 hunks)tools/scripts/storage/init_db.sql(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
src/spider/scheduler/FifoPolicy.cpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)
src/spider/storage/mysql/MySqlStorage.hpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)
src/spider/storage/MetadataStorage.hpp (1)
src/spider/storage/mysql/MySqlStorage.hpp (16)
conn(38-38)conn(39-39)conn(40-40)conn(41-42)conn(43-48)conn(49-55)conn(56-57)conn(58-59)conn(60-61)conn(62-66)conn(67-68)conn(69-73)conn(74-74)conn(75-75)conn(76-77)conn(78-79)
tests/scheduler/test-SchedulerPolicy.cpp (2)
src/spider/core/Data.hpp (1)
gen(40-43)src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)
src/spider/scheduler/scheduler.cpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)
🪛 Cppcheck (2.10-2)
tests/storage/test-MetadataStorage.cpp
[error] 74-74: syntax error
(syntaxError)
src/spider/storage/mysql/MySqlStorage.cpp
[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (30)
src/spider/scheduler/FifoPolicy.hpp (2)
21-21: Appropriate addition of scheduler ID parameterThis change correctly adds a unique scheduler identifier as a parameter to the FifoPolicy constructor, which is essential for the task leasing mechanism being implemented to fix concurrency issues.
35-36: Well-placed member variable for scheduler IDThe addition of this member variable appropriately stores the scheduler's unique identifier, which will be used in the task fetching process to implement the lease mechanism mentioned in the PR objectives.
tests/storage/test-MetadataStorage.cpp (1)
74-74: Test name updated to reflect removed functionalityThe test name has been appropriately updated to "Scheduler addr" since scheduler state management has been removed in favour of the new scheduler leases mechanism.
Note: The static analysis tool flagged a syntax error here, but this appears to be a false positive as the code is syntactically correct.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 74-74: syntax error
(syntaxError)
tests/scheduler/test-SchedulerPolicy.cpp (5)
47-53: Proper scheduler initialization for task leasingGood addition of code to create and register a scheduler with a unique ID before testing the scheduling policy. This matches the changes made to the FifoPolicy constructor and ensures the test works correctly with the new task leasing mechanism.
72-72: Updated policy constructor with scheduler IDThe FifoPolicy constructor call has been correctly updated to include the scheduler_id parameter, aligning with the changes to the class's constructor signature.
117-123: Consistent scheduler setup across test casesGood addition of scheduler initialization code that follows the same pattern as in the first test case, maintaining consistency throughout the tests.
174-180: Maintained consistency in third test caseThe scheduler initialization follows the same pattern as in the other test cases, ensuring consistent test setup across all scenarios.
197-198: Updated policy constructor with scheduler IDThis change correctly updates the FifoPolicy constructor call to include the scheduler_id parameter, consistent with the changes made in the other test cases.
tests/scheduler/test-SchedulerServer.cpp (2)
53-59: Necessary scheduler initialization for server testGood addition of code to initialize and register a scheduler before testing the server functionality. This is required for the new task leasing mechanism and matches the pattern used in the other test files.
60-67: Updated policy constructor with scheduler IDThe FifoPolicy constructor call has been correctly updated to include the scheduler_id as the first parameter, maintaining consistency with the updated class signature.
src/spider/scheduler/FifoPolicy.cpp (3)
19-20: Properly introducing scheduler_id to track task leasesThe addition of the
scheduler_idparameter to the constructor is a good approach for implementing the task lease mechanism described in the PR objectives.
25-26: Initializing new member variable in the appropriate orderGood job adding the
m_scheduler_idinitialization at the beginning of the member initializer list to maintain the declaration order.
67-68: Passing scheduler_id to get_ready_tasks to implement task leasingThe modification to pass the scheduler ID when fetching tasks enables the storage layer to record which tasks are leased by which scheduler, effectively addressing the concurrency bug.
src/spider/storage/mysql/mysql_stmt.hpp (3)
108-115: Good implementation of scheduler_leases table structureThe new table definition is well-structured with appropriate constraints:
- Foreign keys to link scheduler_id and task_id to their respective tables
- CASCADE delete behavior to automatically clean up leases when a scheduler or task is deleted
- Index on scheduler_id for efficient lookups
- Timestamp column with auto-update for tracking lease age
This table structure is essential for the lease mechanism described in the PR objectives, providing a way to track which tasks are leased by which scheduler.
165-165: Array size updated correctly for new table definitionThe
cCreateStoragearray size has been properly increased to include the new table definition.
182-183: Helpful comment about table dependenciesGood inclusion of a comment explaining that the scheduler_lease table must be created after both the scheduler and task tables, which helps maintain the correct order of table creation.
src/spider/storage/MetadataStorage.hpp (2)
86-90: Add scheduler ID parameter looks valid.Introducing
scheduler_idinget_ready_tasksis consistent with the new task lease design. The updated signature properly reflects the need to track which scheduler is leasing tasks.
130-130: Empty line change detected.No functional difference appears here.
src/spider/scheduler/scheduler.cpp (2)
114-114: Signature update for the cleanup loop seems correct.Removing the
metadata_storeparameter in favour of cleaning up only dangling data aligns well with the PR’s new lease-based approach.
217-228: Instantiating the scheduler server with the new FifoPolicy signature.Using the newly introduced
scheduler_id, plus storing it within theFifoPolicyand theSchedulerServer, fits the new leasing logic described in this PR.src/spider/storage/mysql/MySqlStorage.cpp (10)
177-177: Removed thestatecolumn in scheduler insertion.This insertion statement now only includes
id,address, andport. This aligns with eliminating direct scheduler states in favour of leases.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
205-205: Query no longer filters on scheduler state.Retrieving all schedulers without a state filter is consistent with the removal of the
statecolumn from this table.
1229-1230: Confirm lease expiration threshold.Defining
cLeaseExpireTime = 10 msis quite aggressive. Please confirm that a 10 ms timeout is sufficient for real-world scheduling scenarios or consider a more robust duration.
1233-1233: Additional scheduler ID parameter inget_ready_tasks.Requiring the scheduler’s UUID to retrieve tasks helps ensure tasks are distinctly leased to the correct scheduler.
1237-1246: Removing timed-out leases is appropriate.Deleting entries from
scheduler_leasesusingcLeaseExpireTimeensures orphaned leases do not block task scheduling indefinitely.
1254-1254: Excluding leased tasks from the ready tasks query.Ensuring tasks under a lease are not considered ready addresses the concurrency bug of duplicate scheduling.
1286-1304: Conditional skip for tasks already leased.This extra check in the “Get job metadata” flow ensures no duplication if tasks are under lease.
1322-1323: Further restricting tasks to avoid fail/cancel states.This refinement complements the lease-based filtering and clarifies that failed or cancelled tasks are excluded from readiness.
1337-1351: Inserting intoscheduler_leasesupon fetching tasks.Creating leases right after retrieving ready tasks avoids scheduling collisions across multiple schedulers.
1495-1503: Clearing the lease increate_task_instance.Removing the scheduler lease once a new task instance is started closes the loop, preventing indefinite locks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/spider/storage/mysql/MySqlStorage.cpp (1)
1496-1496: Correct the typo in the comment.Change 'scheulder' to 'scheduler'.
- // Remove task from scheulder leases + // Remove task from scheduler leases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/storage/mysql/MySqlStorage.cpp(8 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/storage/mysql/MySqlStorage.cpp
[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (7)
src/spider/storage/mysql/MySqlStorage.cpp (7)
177-177: Implementation correctly removes state management from schedulers table.The SQL query has been updated to remove the
statecolumn, aligning with the schema changes described in the PR where state management has been removed from the schedulers table in favour of using the newscheduler_leasestable.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
205-205: Query correctly updated to remove state filter.The SQL query in
get_active_schedulerhas been modified to remove the condition that filters bystate = 'normal', which is consistent with the removal of the state column from theschedulerstable.
1237-1245: Implementation correctly handles expired leases.The code properly removes expired scheduler leases before fetching new tasks. This ensures that tasks with expired leases can be reassigned to other schedulers if the original scheduler fails to update the task state within the lease time.
1252-1254: Query correctly excludes leased tasks.The SQL query has been updated to exclude tasks that are already leased, which prevents multiple schedulers from processing the same task simultaneously - addressing the concurrency issue described in the PR.
1337-1350: Batch insert implementation for scheduler leases.The code effectively adds all selected tasks to the scheduler leases table using batch processing, which is more efficient than executing individual INSERT statements for each task.
1496-1502: Implementation correctly removes tasks from leases when instances are created.The code properly removes the task from the
scheduler_leasestable when a task instance is created, ensuring that the lease is released once the task is being processed by a worker.
1302-1305: Defensive programming to handle potential inconsistencies.This check ensures that the code gracefully handles situations where a job's metadata is retrieved but its tasks have already been leased to another scheduler. This is a good pattern for handling potential race conditions in a concurrent system.
This reverts commit cc6742e.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/storage/test-MetadataStorage.cpp (1)
542-593: Good implementation of the lease timeout test.This new test case properly validates the task leasing mechanism introduced to fix the concurrency bug in the scheduler. The test confirms that:
- Tasks are correctly leased when first scheduled
- Leased tasks aren't rescheduled immediately
- Tasks can be rescheduled after the lease timeout expires
There are a few minor improvements that could be made:
- Consider checking the return value of
add_scheduleron line 561- Consider checking the return value of
add_jobon line 570- The 2-second sleep duration is hardcoded - consider making this configurable or based on the actual lease timeout setting
- storage->add_scheduler(*conn, spider::core::Scheduler{scheduler_id, "127.0.0.1", 3306}); + REQUIRE(storage->add_scheduler(*conn, spider::core::Scheduler{scheduler_id, "127.0.0.1", 3306}).success()); - storage->add_job(*conn, job_id, gen(), graph); + REQUIRE(storage->add_job(*conn, job_id, gen(), graph).success());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/storage/test-MetadataStorage.cpp(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/storage/test-MetadataStorage.cpp (1)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)
🪛 Cppcheck (2.10-2)
tests/storage/test-MetadataStorage.cpp
[error] 74-74: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (1)
tests/storage/test-MetadataStorage.cpp (1)
74-74: Test case name update looks good.The test case has been renamed from "Scheduler state and addr" to just "Scheduler addr", which reflects the architectural changes in how scheduler states are managed as described in the PR objectives.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 74-74: syntax error
(syntaxError)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/storage/test-MetadataStorage.cpp (1)
542-595: Good implementation of the scheduler lease timeout testThis new test case effectively validates the lease mechanism implementation by:
- Verifying that tasks are leased to a scheduler when first retrieved
- Confirming that leased tasks aren't available for immediate re-scheduling
- Demonstrating that tasks become available again after the lease timeout expires
The test properly validates the core functionality needed to fix the concurrency bug described in the PR objectives.
I would suggest considering one additional enhancement:
+ // Sleep for lease timeout (documented as 2 seconds) std::this_thread::sleep_for(std::chrono::seconds(2));Adding a comment would clarify the expected lease timeout duration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/storage/test-MetadataStorage.cpp(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/storage/test-MetadataStorage.cpp (3)
src/spider/storage/MetadataStorage.hpp (16)
conn(27-27)conn(29-29)conn(30-30)conn(32-33)conn(37-40)conn(42-48)conn(50-51)conn(53-54)conn(56-57)conn(59-63)conn(66-67)conn(69-73)conn(75-75)conn(76-76)conn(77-78)conn(80-80)src/spider/core/TaskGraph.hpp (4)
task(22-38)task(22-22)task(41-48)task(41-41)src/spider/core/TaskGraphImpl.hpp (2)
task(322-344)task(322-322)
🪛 Cppcheck (2.10-2)
tests/storage/test-MetadataStorage.cpp
[error] 74-74: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (1)
tests/storage/test-MetadataStorage.cpp (1)
74-74: Test case name updated to reflect changes in scheduler state managementThe test case name has been appropriately renamed from "Scheduler state and addr" to just "Scheduler addr", reflecting the architectural changes where scheduler state management has been moved from the
schedulerstable to the newscheduler_leasestable.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 74-74: syntax error
(syntaxError)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/scripts/storage/init_db.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
I think this is more of a feat than a fix.
Co-authored-by: davidlion <davidlion2@protonmail.com>
|
This pr is introduced to fix a bug. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/spider/storage/mysql/MySqlStorage.cpp (2)
1229-1229: Lease expiry time remains too short.This concern was raised in a previous review. A 10 ms lease expiry can be too brief for distributed systems. This may not allow enough time for workers to update a task’s state before the scheduler expires the lease and re-queues the task.
1237-1245: Lease timeout logic aligns with concurrency requirements but still relies on a short default.Removing stale leases is a sound approach. However, you are still relying on the 10 ms lease expiry defined above, which may lead to tasks being rescheduled prematurely. Consider making this interval configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/storage/mysql/MySqlStorage.cpp(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (4)
src/spider/storage/mysql/MySqlStorage.cpp (4)
1233-1233: Scheduler ID parameter introduced successfully.The new parameter helps associate tasks with the correct scheduler and improves concurrency handling.
1286-1303: Comment clarifies leased tasks are excluded.Adding this comment improves readability and ensures future maintainers know why some job IDs may not appear in the mapping.
1322-1323: Excluding tasks inscheduler_leases.Filtering out tasks already leased ensures no double-scheduling. This is essential for preventing concurrency races.
1495-1502:Details
❓ Verification inconclusive
Verify multi-scheduler case for lease removal.
This code deletes the lease by task ID only, removing the lease record regardless of which scheduler holds it. If you intend to differentiate by scheduler, you may need to filter on
scheduler_idhere. Otherwise, this approach is correct if tasks are owned globally.Below is a script to examine all lease deletions in the codebase to confirm that removing leases by
task_idalone is intentional:
🏁 Script executed:
#!/bin/bash # Looks for every DELETE statement on scheduler_leases referencing task_id rg "DELETE FROM \`scheduler_leases\`" -C 3Length of output: 1347
Action Required: Verify Scheduler-Specific Lease Deletion
The script output confirms that the lease deletion query in
src/spider/storage/mysql/MySqlStorage.cppat lines 1495–1502 only filters bytask_id, with no reference toscheduler_idanywhere. Please verify that this design is intentional:
- Multi-scheduler Use-Case: Ensure that tasks are indeed globally owned and not expected to be scheduler-specific.
- Potential Update: If different schedulers should only remove their own leases, consider altering the query to filter on
scheduler_id(e.g.,WHERE task_id = ? AND scheduler_id = ?).
sitaowang1998
changed the title
davidlion
changed the title
Description
This pr resolves #105 by adding task leases for schedulers. When a scheduler fetches ready tasks from storage, it writes to the storage all the tasks it just gets, along with the current timestamp, stating the the tasks are leased by the scheduler. These tasks are not considered eligible to be scheduled. The worker removes the lease for the assigned task when updating the task state. If worker fails, we rely on the lease timeout to remove the task from lease, and make them eligible to be scheduled again.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
These changes work together to improve overall backend reliability and responsiveness, ensuring a better experience with task management.