feat(odin): Execute incoming actions from Odin#538
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces custom actions and task control actions (Start/Stop) to the extractor framework, enabling external invocation of tasks and custom targets. It includes registration logic, startup reporting of available actions, multi-threaded action dispatching, and comprehensive unit tests. The review feedback identifies a TOCTOU race condition when starting tasks via actions, recommending atomic check-and-set token registration under a lock. Additionally, it suggests validating that newly added scheduled tasks do not generate action names that conflict with already registered custom actions.
There was a problem hiding this comment.
Code Review
This pull request implements custom action registration and dispatching mechanisms for the Extractor class, allowing users to register custom actions and control scheduled tasks (Start/Stop) externally. It also adds comprehensive unit tests to verify action dispatching and registration lifecycles. The review feedback highlights a critical TOCTOU (Time-of-Check to Time-of-Use) race condition between checking if a task is running and registering its token. The reviewer suggested resolving this by creating and registering the child token atomically under a lock, and provided corresponding updates for the test suite to accommodate the signature changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #538 +/- ##
==========================================
+ Coverage 82.58% 82.73% +0.14%
==========================================
Files 44 44
Lines 4370 4430 +60
==========================================
+ Hits 3609 3665 +56
- Misses 761 765 +4
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for registering and dispatching custom actions and scheduled task control actions (Start/Stop) in the extractor framework, along with comprehensive tests. Feedback highlights a critical concurrency issue where a scheduled task triggered by the scheduler can run concurrently with the same task triggered by an action, potentially overwriting the running task token. Additionally, when a task started via an action is cancelled, the action status incorrectly reports as succeeded instead of cancelled.
Yaseen-A-Khan
left a comment
There was a problem hiding this comment.
I have some comments regarding the changes
|
once team has approved, ping me on slack and I'll do the risk review ASAP =) |
The base branch was changed.
PR Short Summary
Wires the Extractor as the action dispatcher for CheckinWorker and implements the full action routing pipeline: _handle_actions fans out each incoming action to a dedicated daemon thread, _dispatch_single_action routes by matching the action name against registered available actions, and the three handler methods (_handle_start_task_action, _handle_stop_task_action, _handle_custom_action) execute the action and report lifecycle status updates back through the checkin worker.
Type of change
What changed
cognite/extractorutils/unstable/core/base.py
→ _run_task_with_token → succeeded/failed
tests/test_unstable/test_action_dispatch.py — new file, 17 tests covering all acceptance criteria
Why it changed
Odin dispatches pending actions in the checkin response. Without this wiring, actions are received by CheckinWorker but never executed. This completes the round-trip: the extractor announces capabilities at startup (Task 5) and can now act on them.
What to focus on during review
Test evidence
tests/test_unstable/test_action_dispatch.py 17 passed
tests/test_unstable/test_action_registration.py 15 passed
tests/test_unstable/test_actions.py 10 passed
ruff check — All checks passed
Risks and unknowns
Rollout and rollback
Checklist