fix: Move create_task_instance from scheduler to worker - #76
Conversation
WalkthroughThis pull request consolidates task identifier management by transitioning from a tuple-based approach to a single task identifier throughout the code. The Changes
Sequence Diagram(s)sequenceDiagram
participant WC as WorkerClient
participant SS as SchedulerServer
participant DB as MySQL/MetadataStore
WC->>SS: Request next task
SS-->>WC: Return single task_id
WC->>DB: Establish MySQL connection
alt Connection successful
WC->>DB: Create TaskInstance using task_id
DB-->>WC: TaskInstance created (or failure)
else Connection fails
WC->>WC: Log error and return nullopt
end
Possibly related PRs
✨ Finishing Touches
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. 🪧 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: 0
🧹 Nitpick comments (1)
src/spider/worker/WorkerClient.cpp (1)
116-132: Augment error logging for better maintainability.The new logic properly creates the task instance in the worker after retrieving the
task_idfrom the scheduler, consistent with the PR objective. However, ifcreate_task_instancefails, consider logging the specifictask_idand error details to aid in debugging:if (!err.success()) { + spdlog::error("Cannot create task instance for task {}: {}", + boost::uuids::to_string(task_id), + err.description); return std::nullopt; }🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/spider/scheduler/SchedulerMessage.hpp(1 hunks)src/spider/scheduler/SchedulerServer.cpp(1 hunks)src/spider/worker/WorkerClient.cpp(2 hunks)tests/scheduler/test-SchedulerPolicy.cpp(1 hunks)tests/scheduler/test-SchedulerServer.cpp(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/worker/WorkerClient.cpp
[performance] 116-116: 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: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (9)
src/spider/scheduler/SchedulerServer.cpp (1)
162-162: Implementation aligns with PR objective.Setting the response to a single
task_idsimplifies the scheduling flow and correctly delegates task instance creation to the worker side. Ensure that this change is fully propagated to any remaining references of the old tuple approach.tests/scheduler/test-SchedulerServer.cpp (1)
111-111: Verified test logic.This assertion correctly checks the single
task_id, guaranteeing that the server’s simplified response is consistent with the new scheduling approach.tests/scheduler/test-SchedulerPolicy.cpp (3)
69-70: Good modification to allow testing multiple scheduling operationsRemoving
constqualifier fromoptional_task_idcorrectly allows the variable to be reassigned, which is necessary for testing sequential scheduling operations.
77-83: Good test coverage enhancementAdding the test case for scheduling the later task provides better test coverage by ensuring the policy returns tasks in the correct FIFO order. This properly verifies that the second scheduled task (task_2) is returned after the first one.
88-90: Good test coverage for edge caseAdding the test for "Schedule when no task available" appropriately verifies the behavior when there are no more tasks to be scheduled. It ensures that the policy correctly returns an empty optional, which is important for proper error handling in the worker.
src/spider/scheduler/SchedulerMessage.hpp (4)
57-57: Constructor simplified for single task identifier patternThe constructor has been properly simplified to only accept a single task_id, aligning with the PR objective to move the task instance creation responsibility from the scheduler to the worker.
59-64: Interface updated to support single task identifier patternThe methods
has_task_id()andget_task_id()have been correctly modified to work with the new single task identifier approach. This maintains the API contract while simplifying the implementation.
66-66: Serialization updated to match the new data structureThe serialization macro has been correctly updated to only include the single task_id field.
69-69: Member variable updated to simplified modelGood change to replace the tuple-based task identifiers with a single task identifier, which aligns perfectly with the PR's goal of moving task instance creation responsibility to the worker.
1 participant
Description
Scheduler runs
create_task_instance, a storage call, for each schedule task, increasing response delay. This pr moves the call from scheduler to worker.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Refactor
Tests