fix(storage): Always create a reference when getting data from storage (fixes #132). - #119
Conversation
## Walkthrough
This change introduces explicit tracking of job sources (Driver or Task) throughout the job execution and data retrieval workflow. The `Job` class now records its source and associated identifier, which are propagated from both the `Driver` and `TaskContext` classes during job creation. Data retrieval in jobs is updated to use source-specific methods (`get_driver_data` or `get_task_data`) from the `DataStorage` interface, which are implemented in the MySQL storage backend. Function invocation and test code are updated to pass and utilize the task ID, ensuring consistent source-aware data handling.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `src/spider/client/Driver.hpp`, `src/spider/client/TaskContext.hpp` | Updated `start` methods to pass job source (`JobSource::Driver` or `JobSource::Task`) and the relevant source ID (driver or task) to `Job` constructors. |
| `src/spider/client/Job.hpp` | Added `JobSource` enum, new private members for source and source ID, and updated constructors and result retrieval logic to use source-specific data fetching (`get_driver_data` or `get_task_data`). |
| `src/spider/storage/DataStorage.hpp` | Added new pure virtual methods: `get_driver_data` and `get_task_data` to the `DataStorage` interface for source-specific data retrieval. |
| `src/spider/storage/mysql/MySqlStorage.cpp`, `src/spider/storage/mysql/MySqlStorage.hpp` | Implemented `get_driver_data` and `get_task_data` methods, introduced helper for retrieving data with locality, and refactored data retrieval logic to support source-aware access. |
| `src/spider/worker/FunctionManager.hpp`, `src/spider/worker/task_executor.cpp` | Modified function signatures and invocation flow to include and propagate the task ID, and updated data retrieval in function application to use `get_task_data`. |
| `tests/worker/test-FunctionManager.cpp`, `tests/worker/test-TaskExecutor.cpp` | Updated tests to generate, pass, and utilize explicit task IDs in context creation, function invocation, and job submission, ensuring source-aware job and data handling is exercised. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Driver
participant Job
participant DataStorage
participant MySqlDataStorage
Driver->>Job: start(...)\n(JobSource::Driver, driver_id, ...)
Job->>DataStorage: get_result_conn()
alt JobSource::Driver
DataStorage->>MySqlDataStorage: get_driver_data(conn, driver_id, data_id, data)
else JobSource::Task
DataStorage->>MySqlDataStorage: get_task_data(conn, task_id, data_id, data)
end
MySqlDataStorage-->>DataStorage: Data
DataStorage-->>Job: Data
Job-->>Driver: ResultAssessment against linked issues
Possibly related PRs
Suggested reviewers
|
sitaowang1998
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/spider/worker/FunctionManager.hpp (1)
269-276:⚠️ Potential issueLogical operator likely wrong – aborts only when BOTH checks fail
if (msgpack::type::ARRAY != object.type && object.via.array.size < 1)The intent seems to be “type is not ARRAY or the array is empty”.
Using&&means the error path is triggered only when both conditions are true, allowing an empty array to slip through if the type isARRAY.
Replace&&with||:- if (msgpack::type::ARRAY != object.type && object.via.array.size < 1) { + if (msgpack::type::ARRAY != object.type || object.via.array.size < 1) {
🧹 Nitpick comments (2)
src/spider/worker/FunctionManager.hpp (1)
300-324: Potential connection-leak / missing commit
get_storage_factory(...)->provide_storage_connection()returns a unique_ptr that is never explicitlycommit()-ed orrollback()-ed on the success path.
While most DBs autocommit reads, relying on destructor side-effects is fragile and, if the connector starts an implicit transaction, may keep metadata locks longer than necessary.
Consider wrapping the connection usage in an RAII helper that callscommit()once the last argument is parsed.src/spider/storage/mysql/MySqlStorage.cpp (1)
2030-2069: Missingcommit()on successful read
get_data_with_localityreturns after a successful SELECT without committing.
If the connector implicitly opens a transaction, the outer caller must remember to commit/rollback.
Safer pattern:+ static_cast<MySqlConnection&>(conn)->commit(); return StorageErr{};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/spider/client/Driver.hpp(2 hunks)src/spider/client/Job.hpp(4 hunks)src/spider/client/TaskContext.hpp(2 hunks)src/spider/storage/DataStorage.hpp(1 hunks)src/spider/storage/mysql/MySqlStorage.cpp(1 hunks)src/spider/storage/mysql/MySqlStorage.hpp(2 hunks)src/spider/worker/FunctionManager.hpp(4 hunks)src/spider/worker/task_executor.cpp(1 hunks)tests/worker/test-FunctionManager.cpp(8 hunks)tests/worker/test-TaskExecutor.cpp(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/spider/client/Job.hpp (2)
src/spider/client/Driver.cpp (2)
Driver(24-65)Driver(67-106)src/spider/core/Task.hpp (5)
Task(176-179)Task(176-176)Task(181-185)id(187-187)id(187-187)
src/spider/client/Driver.hpp (3)
tests/integration/client.py (1)
Driver(43-44)src/spider/client/Driver.cpp (2)
Driver(24-65)Driver(67-106)src/spider/core/Driver.hpp (2)
Driver(12-12)Driver(12-12)
src/spider/worker/FunctionManager.hpp (2)
src/spider/core/TaskContextImpl.hpp (2)
task_id(16-23)task_id(16-21)src/spider/client/Job.hpp (4)
conn(193-211)conn(193-193)conn(214-376)conn(214-214)
src/spider/storage/mysql/MySqlStorage.cpp (2)
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)src/spider/core/Data.hpp (2)
locality(30-30)locality(30-30)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (25)
src/spider/worker/task_executor.cpp (1)
175-175: Function call updated to include task_id parameter.This change passes the task ID to the function being executed, which is necessary for the new source-aware data retrieval system. This ensures the data accessed during function execution is properly associated with the task.
src/spider/client/TaskContext.hpp (2)
175-182: Job constructor now tracks task as the source.Good implementation - the job is now explicitly initialized with the task as its source, along with the task ID. This ensures proper data reference tracking when retrieving data through this job.
234-241: Task source tracking added to Task Graph jobs.Similar to the previous change, this ensures task graph jobs also track their source as a task with the appropriate task ID, maintaining consistency in the job source tracking implementation.
tests/worker/test-TaskExecutor.cpp (4)
22-23: Added necessary Task-related includes.These additional includes are required for the Task and TaskGraph manipulation in the updated test.
184-195: Properly set up job with explicit task ID for testing.This addition properly creates a job with a valid task ID before testing the task executor, which is essential for testing the new source-aware data retrieval functionality. The test now properly simulates the real-world job creation and execution flow.
206-206: Using explicit task ID in executor creation.Using the same task ID that was used to create the job ensures that the test properly validates the source-aware data retrieval mechanism.
222-222: Added proper cleanup of the test job.Good practice to clean up the job after the test, maintaining test isolation and preventing test data accumulation.
src/spider/storage/DataStorage.hpp (1)
35-49: Added source-specific data retrieval methods.These new methods enhance the DataStorage interface to support retrieving data with source awareness (driver or task). This is the foundation for ensuring data references are properly maintained when data is retrieved, not just when it's created.
Both methods follow the established interface pattern and clearly specify their parameters.
src/spider/client/Driver.hpp (2)
231-232: Good enhancement to propagate source information!These new parameters properly identify the job as originating from a Driver and pass the driver's UUID. This change enables tracking data references during retrieval operations and fixes the issue where data references were only registered during creation.
296-297: Consistent implementation for the TaskGraph version!The same source identification parameters are correctly applied to this overloaded method, ensuring consistent behavior between both start methods. This maintains parity in how source tracking is implemented across the codebase.
src/spider/client/Job.hpp (6)
160-163: Good addition of JobSource enum!This enum clearly identifies the origin of a job (Driver or Task), which is essential for the new data reference tracking system. It provides type safety and clear semantics for the source concept.
165-167: Appropriate constructor enhancement!Updating the constructor to accept source type and ID ensures that every Job instance has the necessary context for proper data reference tracking. This is crucial for addressing the core issue where data references were missing when retrieved.
178-180: Consistent implementation in the overloaded constructor!The same source parameters are correctly added to this constructor, ensuring consistent behavior regardless of how the Job is created.
258-272: Key implementation for source-aware data retrieval!This conditional logic is the core of the fix - it uses the job's source information to call the appropriate data retrieval method, ensuring data references are registered within the same transaction as data retrieval. This directly addresses the issue where data returned to clients lacked references after job deletion.
332-346: Consistent implementation for non-tuple return types!The same source-aware data retrieval logic is correctly implemented for the non-tuple return path, ensuring consistent behavior across all data retrieval scenarios. This maintains the fix for data reference tracking across different types of returned data.
381-382: Essential member variables for tracking source!These member variables store the source information passed to the constructor, enabling the conditional logic in data retrieval methods. They're a necessary part of the solution for tracking data references.
tests/worker/test-FunctionManager.cpp (5)
18-20: Appropriate includes added!Adding includes for Task.hpp and TaskGraph.hpp supports the new task creation and graph manipulation code in the tests, which is needed to validate the source-aware data reference tracking.
81-82: Good change to use consistent task ID!Generating a single task ID per test case ensures consistency throughout the test and properly validates that source information is correctly propagated across function calls.
101-101: Updated function call with task ID parameter!This change passes the task ID to the function, matching the updated signature and testing the propagation of source information through the function invocation path.
187-198: Excellent test setup for data reference validation!This code properly creates a task with the test task ID, associates it with input data, builds a task graph, and submits it as a job. This comprehensive setup validates the end-to-end flow of the source-aware data reference tracking system, particularly ensuring that data is correctly associated with its source task.
214-214: Good test cleanup!Adding cleanup code to remove the job ensures proper test isolation and prevents resource leakage between test cases.
src/spider/storage/mysql/MySqlStorage.hpp (2)
151-162: Excellent implementation of source-specific data retrieval!These new methods implement the core storage functionality needed to fix the data reference issue. They retrieve data while registering references to either a driver or task within the same transaction, ensuring data references are properly maintained even when jobs are deleted.
The method signatures correctly take source IDs (driver_id or task_id) to associate with the data, which is essential for the source-aware data reference tracking.
202-204: Good helper method for code reuse!This private helper method likely handles the common functionality needed by both source-specific retrieval methods, promoting code reuse while maintaining clear separation of concerns. It retrieves data with locality information, which is crucial for the storage implementation.
src/spider/worker/FunctionManager.hpp (2)
377-382: Placeholder order looks correct – nice catchThe extra
_2placeholder fortask_idis inserted in the right spot, keeping the ABI stable for callers.
47-49:Details
❓ Verification inconclusive
Verify all call-sites after changing the
Functionsignature
Functionnow expects(TaskContext&, boost::uuids::uuid, ArgsBuffer const&).
Any older invocations that still pass only two arguments will fail to compile or, worse, be implicitly converted to an unintended overload.
Please do a quick grep across the repo to confirm everyFunction(and anystd::functionyou create from it) is invoked with three parameters.Run:
🏁 Script executed:
#!/bin/bash rg -n "get_function(" | rg -v "task_id"Length of output: 116
To avoid the regex error and reliably find all
get_function(calls missing thetask_idparameter, let’s search using a fixed-string match:
🏁 Script executed:
#!/bin/bash # Search for get_function( without accidentally treating parentheses as regex rg -n -F "get_function(" | rg -v "task_id"Length of output: 892
We need to inspect how these functions are actually invoked. Let’s dump the surrounding lines in both the test and the executor:
🏁 Script executed:
#!/bin/bash # Show context around get_function in the tests rg -n -C3 "manager.get_function" tests/worker/test-FunctionManager.cpp # Show context around get_function in task_executor rg -n -C3 "get_function" src/spider/worker/task_executor.cppLength of output: 2210
Let’s inspect the actual invocation in
task_executor.cppto ensure it’s called with all three args:
🏁 Script executed:
#!/bin/bash # Show more context around get_function in the executor rg -n -C6 "get_function" src/spider/worker/task_executor.cpp # Look for the actual call to (*function) there rg -n -C6 "(*function)" src/spider/worker/task_executor.cppLength of output: 870
Verify all call-sites include the new
task_idparameterAutomated grep confirms that in
tests/worker/test-FunctionManager.cppthe calls to(*function)pass three arguments (context, task_id, args_buffers), but we couldn’t locate the actual invocation in the executor. Please manually inspect:• src/spider/worker/task_executor.cpp – ensure the
Function const* function = …;is invoked as
(*function)(context, task_id, args_buffer)(or equivalent) with all three parameters.
• Any other sites where aFunctionpointer orstd::functionis called, verify they’re updated to accept(TaskContext&, boost::uuids::uuid, ArgsBuffer const&).
Co-authored-by: davidlion <davidlion2@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/spider/storage/mysql/MySqlStorage.cpp (1)
2163-2191: Consider handling duplicate key errors gracefully for task referencesSimilar to the driver references, the task reference insertion should handle duplicate keys gracefully to avoid unnecessary transaction rollbacks.
Consider modifying the SQL to use ON DUPLICATE KEY UPDATE or checking if the reference exists before inserting:
- std::unique_ptr<sql::PreparedStatement> statement{ - static_cast<MySqlConnection&>(conn)->prepareStatement( - "INSERT INTO `data_ref_task` (`id`, `task_id`) VALUES (?, ?)" - ) - }; + std::unique_ptr<sql::PreparedStatement> statement{ + static_cast<MySqlConnection&>(conn)->prepareStatement( + "INSERT IGNORE INTO `data_ref_task` (`id`, `task_id`) VALUES (?, ?)" + ) + };Alternatively, in the catch block:
} catch (sql::SQLException& e) { static_cast<MySqlConnection&>(conn)->rollback(); + if (e.getErrorCode() == ErDupKey || e.getErrorCode() == ErDupEntry) { + // Reference already exists, this is fine + static_cast<MySqlConnection&>(conn)->commit(); + return StorageErr{}; + } return StorageErr{StorageErrType::OtherErr, e.what()}; }
🧹 Nitpick comments (1)
src/spider/storage/mysql/MySqlStorage.cpp (1)
2133-2161: Consider handling duplicate key errors gracefullyThe method correctly implements driver reference tracking, but it could handle the case where the same reference already exists more gracefully. Currently, a duplicate key error would cause a transaction rollback, which might be inefficient.
Consider modifying the SQL to use ON DUPLICATE KEY UPDATE or checking if the reference exists before inserting:
- std::unique_ptr<sql::PreparedStatement> statement{ - static_cast<MySqlConnection&>(conn)->prepareStatement( - "INSERT INTO `data_ref_driver` (`id`, `driver_id`) VALUES (?, ?)" - ) - }; + std::unique_ptr<sql::PreparedStatement> statement{ + static_cast<MySqlConnection&>(conn)->prepareStatement( + "INSERT IGNORE INTO `data_ref_driver` (`id`, `driver_id`) VALUES (?, ?)" + ) + };Alternatively, in the catch block:
} catch (sql::SQLException& e) { static_cast<MySqlConnection&>(conn)->rollback(); + if (e.getErrorCode() == ErDupKey || e.getErrorCode() == ErDupEntry) { + // Reference already exists, this is fine + static_cast<MySqlConnection&>(conn)->commit(); + return StorageErr{}; + } return StorageErr{StorageErrType::OtherErr, e.what()}; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/spider/client/Job.hpp(4 hunks)src/spider/storage/DataStorage.hpp(1 hunks)src/spider/storage/mysql/MySqlStorage.cpp(1 hunks)src/spider/storage/mysql/MySqlStorage.hpp(2 hunks)src/spider/worker/FunctionManager.hpp(4 hunks)tests/worker/test-FunctionManager.cpp(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/spider/storage/mysql/MySqlStorage.hpp
- tests/worker/test-FunctionManager.cpp
- src/spider/storage/DataStorage.hpp
- src/spider/client/Job.hpp
- src/spider/worker/FunctionManager.hpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/spider/storage/mysql/MySqlStorage.cpp (3)
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)src/spider/core/Task.hpp (6)
id(187-187)id(187-187)locality(156-158)locality(156-156)locality(160-162)locality(160-160)src/spider/core/Data.hpp (2)
locality(30-30)locality(30-30)
🔇 Additional comments (2)
src/spider/storage/mysql/MySqlStorage.cpp (2)
2077-2116: Looks good: New helper method for data retrieval with localityThis helper method properly fetches data by ID and retrieves its associated localities in a structured way. It correctly handles the case where the data isn't found and appropriately populates the provided Data object.
2118-2131: Good refactoring of get_data to use the new helperThe refactoring of the existing
get_datamethod to use the new helper is clean and maintains proper transaction handling.
|
@CodeRabbit. Please create an issue about cleaning up storage functions that are used only in testing. Please create another issue about moving the data fetching from |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19357 🏁 Script executed: Length of output: 15295 cpp 🏁 Script executed: Length of output: 1711 🏁 Script executed: Length of output: 1620 @sitaowang1998 I've created both issues for you:
Both issues have been tagged with your username for follow-up. |
davidlion
changed the title
2 participants
Description
Problem
Spider currently only tracks
Datareferences when they are created by a task or client. However, if a job returns aDataobject to a client, Spider does not register that the client is referencing thisData. As a result, when the job is removed from storage, theDatamay be garbage collected, even though the client is still referencing , leading to potential errors.Solution
This PR introduces new storage functions that retrieve
Dataand register a reference to it within the same transaction. All existing calls toget_datahave been replaced with these new functions to ensure proper tracking of Data references. The associated tests have also been updated to reflect and validate the new behavior.Fixes #132.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit