feat: Add task context implementation - #44
Conversation
WalkthroughThis pull request introduces significant enhancements to the Spider framework's task management and execution infrastructure. The changes focus on improving function name management, task context handling, and type safety across various components. Key modifications include adding new source files for Changes
Sequence DiagramsequenceDiagram
participant Client
participant TaskContext
participant FunctionNameManager
participant MetadataStorage
Client->>TaskContext: Create with task ID
TaskContext->>FunctionNameManager: Register function name
TaskContext->>MetadataStorage: Set task running
Client->>TaskContext: Start task
TaskContext-->>Client: Return Job
Possibly related PRs
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: 4
🧹 Nitpick comments (25)
tests/worker/worker-test.hpp (1)
17-18: Ensure create_task_test is validated under diverse scenarios.
Consider adding negative and boundary tests to confirm that the implementation handles unexpected inputs and edge cases.src/spider/worker/FunctionNameManager.cpp (1)
10-15: Return early with std::nullopt maintains correctness.
The function name retrieval logic is straightforward. Ensure no stale pointers remain inm_name_map.tests/integration/test_client.py (1)
35-37: Consider parameterizing worker commands for further scalability.
Duplicating worker commands works here but parameterizing them might improve manageability for future expansions.src/spider/worker/FunctionNameManager.hpp (1)
38-42: Consider returning more descriptive error states rather than a Boolean.
Returning only a Boolean might be insufficient for debugging or auditing function registration failures.src/spider/storage/MetadataStorage.hpp (1)
57-57: Add documentation for task state transitions.
Theset_task_runningmethod should clearly specify permitted transitions (e.g., from "ready" to "running") and define behaviour for invalid transitions.tests/client/client-test.cpp (1)
112-140: Increase validation coverage for the new jobs.
Consider adding tests for boundary cases and error paths, such as invalid parameters, to strengthen the reliability ofcreate_data_jobandcreate_task_job.src/spider/core/Task.hpp (2)
15-16: CheckData.hppusage
Adding this include may introduce dependencies that can affect build times or overshadow other definitions. Confirm it is necessary and does not cause side effects.
28-30: Confirm shift from user-supplied to fixed type
Use oftypeid(spider::core::Data).name()might produce compiler-dependent strings and replaces the user's ability to specify a custom type. If you intend for the type to remain customizable, consider providing another overload that accepts a type parameter.src/spider/core/TaskGraph.hpp (1)
160-161: Retain clarity with hashed map
Including the custom hash is valid, but be mindful of the potential for collisions. Document the reason for usingstd::hash<boost::uuids::uuid>in the code comments if it’s essential.src/spider/worker/TaskExecutor.hpp (1)
43-43: Consider empty or invalid UUID
Adding thetask_idparameter is helpful, but ensure invalid or default-constructed UUIDs are not passed in. Consider adding a precondition or guard check.src/spider/storage/MysqlStorage.hpp (1)
59-59: Consider concurrency implications and add documentation.The new method
set_task_runningis straightforward. Before integrating code that changes task states, ensure that there is no concurrent state racing, such as a scenario where a task is set to running while another operation tries to reset or finalize the same task. Providing documentation for the state transition rules would help avoid future confusion.src/spider/worker/task_executor.cpp (4)
40-44: Document the new command-line argument.Adding the
--task_idargument is helpful for clarifying which task is being executed. Ensure the help text is updated or expanded to inform users about the format (e.g., a valid UUID) they must provide.
88-88: Variable declared but not used until lines below.While declaring
std::string task_id_string;is valid, consider deferring it closer to where it is used or ensuring immediate usage to reduce confusion.
97-97: Avoid repeating logic.
task_id_string = args["task_id"].as<std::string>();appears twice. If the code is intended to run again under certain conditions, reconsider refactoring to reduce duplication and potential drift between duplication points.🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
122-125: Handle parsing exceptions carefully.Parsing a UUID from an unverified string could raise exceptions. Ensure try-catch blocks are robust enough to handle malformed UUID strings gracefully.
tests/worker/test-FunctionManager.cpp (1)
120-122: Validate thorough coverage of tuple returns.Creating the
TaskContextwith a random UUID is consistent with the rest of the code. Confirm that your test thoroughly exercises tuple-based responses, including edge cases (e.g., empty strings, zero integers).src/spider/client/Driver.hpp (1)
34-35: Consider wrapping multiple macro expansions
The updated macro appendsSPIDER_WORKER_REGISTER_TASK_NAME(func)toSPIDER_WORKER_REGISTER_TASK(func). If these expansions produce multiple statements, you might wish to wrap them in a do-while block to avoid potential warnings or unexpected behaviour in certain compilers.-#define SPIDER_REGISTER_TASK(func) \ - SPIDER_WORKER_REGISTER_TASK(func) SPIDER_WORKER_REGISTER_TASK_NAME(func) +#define SPIDER_REGISTER_TASK(func) \ + do { \ + SPIDER_WORKER_REGISTER_TASK(func); \ + SPIDER_WORKER_REGISTER_TASK_NAME(func); \ + } while(0)src/spider/client/Job.hpp (2)
175-177: Typeid check for T
Same note applies: verifying output type withtypeid(T).name()is valid if you rely on consistent compiler behaviour.
278-278: Friendship with TaskContext
GrantingTaskContextfriend access may be necessary. Ensure no inadvertent coupling emerges.Would you like to explore alternative designs that do not require friend access?
src/spider/worker/worker.cpp (3)
161-164: Sufficient error logging
Logging the task function name and ID with the missing input index offers clarity for debugging. Consider also capturing job ID if relevant.
229-231: Double-check logging level
When failing to set a task to running, consider logging at a higher severity thandebugif it indicates a potential operational issue.- spdlog::debug("Failed to update task status to running: {}", err.description); + spdlog::warn("Failed to update task status to running: {}", err.description);
246-249: Clarify error context
When callingtask_fail, specifying “parse arguments” in the error message (instead of “parse results”) would better reflect that the failure occurred while retrieving the arguments.- fmt::format("Task {} failed to parse results", task.get_function_name()) + fmt::format("Task {} failed to parse arguments", task.get_function_name())tests/worker/worker-test.cpp (2)
40-43: Consider adding error handling for data builder operations.While the implementation is clean and follows the builder pattern correctly, it might benefit from error handling to gracefully handle potential failures during data construction.
auto create_data_test(spider::TaskContext& context, int x) -> spider::Data<int> { + try { spider::Data<int> data = context.get_data_builder<int>().build(x); return data; + } catch (const std::exception& e) { + std::cerr << "Failed to build data: " << e.what() << "\n"; + throw; + } }
45-57: Enhance logging mechanism and simplify task binding.Two suggestions for improvement:
- Consider using a proper logging framework instead of std::cerr
- The binding of sum_test to itself appears redundant
auto create_task_test(spider::TaskContext& context, int x, int y) -> int { - spider::TaskGraph const graph = context.bind(&sum_test, &sum_test, 0); + spider::TaskGraph const graph = context.bind(&sum_test); - std::cerr << "Create task test\n"; - spider::Job job = context.start(graph, x, y); - std::cerr << "Job started\n"; + spider::Job job = context.start(graph, x, y); job.wait_complete(); - std::cerr << "Job completed\n"; if (job.get_status() != spider::JobStatus::Succeeded) { - std::cerr << "Job failed\n"; throw std::runtime_error("Job failed"); } return job.get_result(); }tests/worker/test-TaskExecutor.cpp (1)
66-67: LGTM! Good test isolation with unique task IDs.The implementation correctly uses random UUIDs for each test case, ensuring proper test isolation. Consider creating a test fixture to reduce code duplication of the UUID generator setup.
class TaskExecutorTest { protected: boost::uuids::random_generator gen; boost::uuids::uuid getUniqueTaskId() { return gen(); } }; TEST_CASE_METHOD(TaskExecutorTest, "Task execute success", "[worker][storage]") { // ... use getUniqueTaskId() instead of gen() }Also applies to: 71-71, 94-95, 99-99, 120-121, 125-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/spider/CMakeLists.txt(3 hunks)src/spider/client/Driver.hpp(4 hunks)src/spider/client/Job.hpp(6 hunks)src/spider/client/TaskContext.cpp(1 hunks)src/spider/client/TaskContext.hpp(4 hunks)src/spider/core/DataImpl.hpp(1 hunks)src/spider/core/Task.hpp(3 hunks)src/spider/core/TaskContextImpl.hpp(2 hunks)src/spider/core/TaskGraph.hpp(3 hunks)src/spider/core/TaskGraphImpl.hpp(8 hunks)src/spider/storage/MetadataStorage.hpp(1 hunks)src/spider/storage/MysqlStorage.cpp(16 hunks)src/spider/storage/MysqlStorage.hpp(1 hunks)src/spider/worker/DllLoader.cpp(2 hunks)src/spider/worker/FunctionManager.cpp(0 hunks)src/spider/worker/FunctionManager.hpp(5 hunks)src/spider/worker/FunctionNameManager.cpp(1 hunks)src/spider/worker/FunctionNameManager.hpp(1 hunks)src/spider/worker/TaskExecutor.hpp(5 hunks)src/spider/worker/task_executor.cpp(5 hunks)src/spider/worker/worker.cpp(6 hunks)tests/client/client-test.cpp(1 hunks)tests/integration/test_client.py(3 hunks)tests/scheduler/test-SchedulerPolicy.cpp(2 hunks)tests/utils/CoreTaskUtils.cpp(2 hunks)tests/worker/test-FunctionManager.cpp(5 hunks)tests/worker/test-TaskExecutor.cpp(4 hunks)tests/worker/worker-test.cpp(2 hunks)tests/worker/worker-test.hpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/spider/worker/FunctionManager.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
tests/worker/test-FunctionManager.cpp
[error] 47-47: syntax error
(syntaxError)
src/spider/worker/task_executor.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (94)
src/spider/client/TaskContext.cpp (4)
15-17: Looks good: retrieving task ID is straightforward.
Implementation is succinct and correct.
19-29: Key-value store retrieval is well-structured.
Optional usage is concise, with appropriate exception handling.
31-37: Key-value store insertion is well-implemented.
Error handling via exceptions is consistent with the rest of the code.
39-46: Fetching job IDs looks correct.
Proper exception is thrown upon failure, which meets expectations.src/spider/client/TaskContext.hpp (7)
6-6: Header inclusions appear relevant.
No duplication or superfluous headers. Nicely done.Also applies to: 8-8, 9-9, 13-13, 15-15, 17-17, 18-18, 22-22
58-61: Builder method is concise.
This approach integrates the data store and task ID seamlessly.
98-110: The 'bind' method is well-structured.
Input parameters are validated appropriately, with clear error handling.
123-160: The 'start' method for tasks demonstrates solid input validation.
Ensures consistent type checks and meaningful exception handling.
167-204: The 'start' method for task graphs parallels the task flow effectively.
Maintains consistent checks and error handling while resetting graph IDs.
218-223: Updated TaskContext constructor supports task-specific IDs.
This addition properly addresses scoping for identity management.
230-231: New private member 'm_task_id' is well-introduced.
Task identification aligns with the constructor changes.src/spider/core/TaskGraphImpl.hpp (13)
21-21: Replacing 'FunctionManager.hpp' with 'FunctionNameManager.hpp' looks correct.
This reflects the new approach to function naming.
24-24: Forward declaration of 'class Data'.
Ensures minimal header dependencies and clarity.
60-66: Parent task integration appears clean.
Error checking ensures the graph only adds valid tasks and dependencies.
79-82: Adding tasks from the parent graph maintains consistent checks.
Good practice resetting IDs to avoid collisions.
98-101: Data type verification for 'spider::Data'.
Helps maintain strong type safety for task inputs.
138-138: Using 'FunctionNameManager' to retrieve function names is consistent with the new architecture.
Streamlines referencing across the codebase.
147-155: Adding task inputs by type is well done.
Ensures each input is aligned with the declared type in the function signature.
159-164: Handling multiple tuple outputs is thorough.
Each tuple element is declared as a separate output, maintaining type safety.
167-171: Single output tasks are handled consistently.
Mirrors the logic applied to tuple outputs.
193-196: Verifying 'spider::Data' in 'task_add_input'.
Helps avoid inconsistencies in data usage.
199-202: Serializing non-'Data' parameters is correct.
String-based storage is straightforward and flexible.
207-208: Fallback path is minimal.
Returns 'fail' if the type is neither 'Data' nor a serializable type.
249-260: 'add_inputs' method gracefully handles task input matching.
Sequentially retrieves tasks, checks types, and sets data or serialized values.src/spider/worker/FunctionManager.hpp (4)
47-47: Switching to pass TaskContext by reference is a sensible improvement.
Mitigates unnecessary copies and potential overhead.
203-213: Creating a result buffer for a single return value.
The approach cleanly differentiates between 'Data' and generic types.
217-231: Tuple-based result responses appear well-implemented.
Properly unrolls each element, including 'Data' objects.
275-275: Passing TaskContext by reference in 'apply' is consistent with new function signatures.
Reduces overhead and ensures shared state is maintained properly.tests/worker/worker-test.hpp (1)
15-16: Add test coverage for create_data_test.
It is advisable to include unit tests for this newly introduced function to ensure correctness and maintain thorough coverage.src/spider/worker/FunctionNameManager.cpp (3)
1-4: Include necessary concurrency safeguards if used in multi-threaded contexts.
While these includes seem appropriate, confirm thatm_name_mapaccess is properly synchronized when code is run concurrently.
6-9: Boost DLL alias usage is appropriate.
Keeping these includes separate aids clarity. No immediate concerns here.
19-23: Alias naming clarity.
Confirm that the alias namefunction_name_manager_get_instanceis sufficiently descriptive for downstream usage.src/spider/core/TaskContextImpl.hpp (2)
6-7: New #include for boost::uuid is relevant.
No immediate issues. Confirm that the library remains consistent with other parts of the codebase.
16-20: Ensure the new task_id parameter is always populated.
When callingcreate_task_context, verify that a valid UUID is provided to avoid tracking issues later.tests/integration/test_client.py (3)
14-16: Renaming to start_scheduler_workers adds clarity.
Great improvement. The multi-worker setup is well represented in the updated function name.
45-53: Tear-down updates reflect multi-worker usage.
Killing individual processes is crucial. No concerns here, but ensure you handle any edge cases where a process might already have exited.
65-65: Adding a 20-second timeout is beneficial.
This helps avoid indefinite hangs and ensures faster feedback during testing.src/spider/worker/FunctionNameManager.hpp (1)
44-44: Clarify concurrency guarantees forget_function_name.
It is unclear if simultaneous calls to this method or concurrent updates to the map are safe. Document or enforce threading guarantees to avoid data races.src/spider/worker/DllLoader.cpp (1)
40-53: Verify repeated registration logic.
When loading multiple shared libraries, ensure that repeated calls toregister_functionwith the same function pointer do not cause conflicts or unexpected behaviour.tests/utils/CoreTaskUtils.cpp (2)
92-96: Ensure consistent hash usage
It is good to see you specifying a custom hash parameter. However, confirm that this usage is consistent across your codebase and consider if you need specialized collision checks or performance tuning for UUID hashing.
119-119: Confirmtask_equalalignment
The invocation ofhash_map_equalusingtask_equalis correct. Verify that thetask_equalfunction aligns with any new fields introduced incore::Task.src/spider/core/Task.hpp (1)
69-71: Discuss constructor forTaskOutput
Similar toTaskInput, consider whether you need an overload that allows calling code to supply a distinct type name. Relying ontypeidmight result in an unstable string identifier across different compilers.src/spider/core/TaskGraph.hpp (2)
90-92: Consistent Hash Usage forget_tasks
Using a custom hash type for UUID ensures consistency with the rest of the code. Just ensure that all related UUID operations use the same hash function for uniformity.
123-124: Validate new mapping inreset_ids
You replaced the existing map withnew_tasksusing the hashed UUID key. Confirm all references remain valid and that tasks are re-indexed properly.src/spider/worker/TaskExecutor.hpp (2)
53-61: Include--task_idin process arguments
This is a good way to expose the task identifier. Validate that downstream code properly interprets the UUID and that collisions are unlikely when multiple tasks run simultaneously.
101-109: Forwardtask_idconsistently
It’s good that you consistently forwardtask_idin your process arguments. If you plan to expand on task-specific logic, confirm that your parsing logic handles malformed UUID strings.tests/scheduler/test-SchedulerPolicy.cpp (2)
99-99: Confirm constructor parameter removal is valid.By removing the second constructor parameter from
TaskInput, the usage is now streamlined. Confirm that the default initialisation aligns with the intended data type or metadata, especially if any part of your scheduling logic previously depended on that second parameter.
146-146: Validate consistency of task inputs.The same removal of the second constructor parameter applies here. Ensure that the rest of the test logic does not rely on the removed detail or type tracking.
src/spider/worker/task_executor.cpp (4)
16-17: Check for consistent usage of new headers.The new includes for
<boost/uuid/string_generator.hpp>and<boost/uuid/uuid.hpp>are correct. Just ensure that they are consistently referenced throughout the code for parsing and working with UUIDs, and that there is no duplication of imports elsewhere.
94-96: Ensure consistent error handling for missing arguments.Your code correctly checks for
task_idin the arguments. Since it returnscCmdArgParseErrif absent, verify that the surrounding logic gracefully handles early termination.
101-101: Duplicate assignment observation.This repeats the assignment to
task_id_string. Confirm if this duplication is intentional or if you can consolidate it with the logic on line 97 for clarity.
179-183: InstantiateTaskContextwith broad error boundaries.Constructing
TaskContextwithtask_idis a sound approach. Confirm that external calls (like to storage) handle missing or invalid IDs gracefully, especially if the executor receives an unexpected or corrupted task identifier.tests/worker/test-FunctionManager.cpp (5)
19-19: Ensure the new header aligns with function name usage.Including
"FunctionNameManager.hpp"clarifies the usage of separate registries for tasks. Ensure the olderFunctionManagerreferences are updated to avoid confusion about their responsibilities.
43-45: Check coverage of the new registration macros.The macros
SPIDER_WORKER_REGISTER_TASK_NAMEforint_test,tuple_ret_test, anddata_testlook correct. Ensure adequate tests confirm that the new function name registry is functioning as expected.
48-49: Confirm thread-safety for concurrent name lookups.When retrieving function names from the
FunctionNameManager, be sure the manager handles concurrent lookups or updates if used in multi-threaded contexts (e.g., parallel test execution).
67-69: Ensure unique context creation usage.The usage of a random generator for each
TaskContextis a good approach to ensure uniqueness. Keep in mind that the random generator may produce collisions in extremely large-scale systems.
160-164: Confirm data removal and cleanup.After testing with
data_test, you remove the data from storage. Verify that theTaskContextno longer needs references to the data once the function completes, ensuring no lingering references remain.src/spider/client/Driver.hpp (3)
21-21: Include header is appropriate
This new include ofFunctionNameManager.hppis required for naming tasks in the updated macro.
156-159: Compile-time checks for input types
Using static_assert to enforce matching parameter types at compile time is a robust approach.
206-209: Reinforce the compile-time checks
This static_assert similarly enhances safety by confirming parameter-type alignment for the task graph inputs. Good practise for ensuring correctness at compile time.src/spider/client/Job.hpp (4)
36-36: Forward declaration is beneficial
Forward declaringTaskContexthere reduces header dependencies and improves compile times.
155-157: Beware of compiler-specific typeinfo
Usingtypeid(core::Data).name()for comparison might yield different encoded names across compilers. Nonetheless, if your deployment environment is consistent, the approach is acceptable.
213-215: Data output mismatch
Raising an exception for unexpected output types helps identify logic errors early.
232-234: Runtime type checking
ThrowingConnectionExceptionon a type mismatch clarifies the nature of the error for the caller.src/spider/worker/worker.cpp (2)
143-145: Index-based iteration is clear
Iterating using the index helps produce more precise logs and is simpler to read for multi-step logic.
258-258: Pass ID to TaskExecutor
Providing the task ID to theTaskExecutorhelps associate logs and results with the correct task. This is a positive step for improved traceability.src/spider/storage/MysqlStorage.cpp (25)
323-327: Consistent string conversion
Centralizingsql::SQLStringconversion inget_sql_stringprovides consistency and avoids repeated inline conversions.
393-393: Apply get_sql_string
Switching toget_sql_stringfor the driver address retrieval improves consistency across the codebase.
411-411: Consistent usage
Applyingget_sql_stringagain ensures uniform handling of SQL results.
529-532: Clear error message
Returning aKeyNotFoundErrwith a textual explanation ensures clarity for higher-level callers.
636-637: Reusing conversion method
Retrievingfunc_nameandstateby the new method ensures uniform string parsing.
644-644: Input type retrieval
Converting the string type at this stage continues the standardization.
648-648: Input value extraction
Replacing direct c_str usage withget_sql_stringis safer and more maintainable.
655-655: Inline usage
Constructing aTaskInputfrom the extracted string is consistent with prior changes.
657-657: Handle binary stream
Reading the data from the binary stream for the new input path is coherent with the function’s logic.
664-664: Output type retrieval
Continuing the pattern for the output type fosters code uniformity.
667-667: Setting output value
Using the helper function to retrieve a string is straightforward and consistent.
679-679: Use for tasks
Again, retrieving the string for the type ensures a standardized approach.
688-688: Set input value
Applying the new method inside the loop is consistent with the rest of the changes.
695-695: Tuple input usage
Storing the input withget_sql_stringhelps keep string data robust.
697-697: Binary read improvement
Usingread_idfor the binary stream continues the uniform approach for input tasks.
712-712: Identical approach
Same pattern for retrieving task output type. Maintains clarity across all usage sites.
715-715: Output set_value
Making the code consistent everywhere fosters easier maintenance.
899-906: Timestamp conversion
The parse failure message is explicit, helpful in diagnosing invalid date/time formats.
1212-1230: set_task_running logic
The new method transitions a task from ‘ready’ to ‘running’ and rolls back if the task is not in the correct state. This approach enforces concurrency control.
1504-1504: Scheduler state
Retrieving the scheduler state with the helper function keeps the code uniform.
1542-1542: Driver address extraction
Keeping address retrieval consistent with previous usage is excellent for maintainability.
1713-1713: Set Data value
Replacing direct string usage withget_sql_stringensures all textual data flows through the same utility.
1723-1723: Locality vector population
Fetching and inserting addresses with consistent patterns eliminates confusion.
1950-1950: client_kv_data retrieval
Providing a unified string conversion for the returned result is beneficial.
1985-1985: task_kv_data retrieval
Likewise, retrieving the string value withget_sql_stringunifies all code paths.tests/worker/worker-test.cpp (2)
9-11: LGTM! Required includes for task context implementation.The new includes are properly ordered and necessary for the task context functionality.
64-65: LGTM! Proper task registration.The new tasks are correctly registered using the SPIDER_REGISTER_TASK macro within the NOLINT block.
src/spider/CMakeLists.txt (1)
5-5: LGTM! Proper integration of new components.The new source and header files are correctly added to their respective target lists, ensuring proper build integration.
Also applies to: 28-28, 129-129
| if constexpr (cIsSpecializationV<T, spider::Data>) { | ||
| static_assert("Not implemented"); | ||
| return std::make_optional(object.via.array.ptr[1].as<T>().get_id()); | ||
| } else { |
There was a problem hiding this comment.
'static_assert("Not implemented")' might block compilation.
Consider either implementing or removing the 'Data' handling logic here.
| using T = std::tuple_element_t<i.cValue, std::tuple<Ts...>>; | ||
| if constexpr (cIsSpecializationV<T, spider::Data>) { | ||
| static_assert("Not implemented"); | ||
| } |
There was a problem hiding this comment.
Unhandled 'spider::Data' in multi-value results.
Again, 'static_assert("Not implemented")' will prevent usage.
|
|
||
| template <class T> | ||
| static auto get_impl(spider::Data<T> const& data) -> std::shared_ptr<DataStorage> { | ||
| static auto get_impl(spider::Data<T> const& data) -> std::unique_ptr<Data> const& { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returning a 'std::unique_ptr const&' can be risky.
A reference to a unique_ptr might imply shared ownership. Consider returning a raw pointer or a shared_ptr instead for clarity.
| TaskExecutor( | ||
| boost::asio::io_context& context, | ||
| std::string const& func_name, | ||
| boost::uuids::uuid const task_id, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm constructor parameter coverage
Ensure that the second constructor also gracefully handles edge cases for task_id. You may want to unify constructor logic and reduce code duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/spider/worker/task_executor.cpp (1)
40-44: Enhance the task_id parameter description.While the implementation is correct, consider making the description more specific about the expected UUID format to help users provide valid input.
- "task id of the function" + "task id of the function (must be a valid UUID string)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/spider/worker/task_executor.cpp(5 hunks)src/spider/worker/worker.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spider/worker/worker.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/worker/task_executor.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (3)
src/spider/worker/task_executor.cpp (3)
16-17: LGTM! Appropriate includes for UUID handling.The new Boost UUID headers are correctly placed and necessary for the task ID functionality.
88-88: LGTM! Consistent error handling for task_id.The task ID validation and error handling follow the established pattern for required arguments.
Also applies to: 94-97
178-182: LGTM! Verify task context implementation across the codebase.The TaskContext creation is correctly implemented with the new task_id parameter. Let's verify the implementation across related files.
✅ Verification successful
Let me gather more information about the TaskContext implementation.
TaskContext implementation is consistent across the codebase
The verification shows that the TaskContext implementation is consistent and properly integrated:
- The TaskContext constructor correctly accepts task_id parameter and stores it in the member variable
- The create_task_context factory method is consistently used across the codebase with the required task_id parameter
- All test cases properly pass task_id when creating TaskContext instances
- The task_id is properly used throughout the storage layer for task tracking and management
- Database schema and queries properly handle task_id relationships
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TaskContext implementation consistency # Check TaskContext constructor implementations echo "Checking TaskContext implementations..." ast-grep --pattern 'class TaskContext { $$$ TaskContext($$$task_id$$$) { $$$ } $$$ }' # Check create_task_context usage echo "Checking create_task_context usage..." rg "create_task_context" -A 3Length of output: 1977
Script:
#!/bin/bash # Check TaskContext interface and implementation details echo "Checking TaskContext interface..." rg "class TaskContext" -A 10 echo -e "\nChecking TaskContextImpl implementation..." rg "class TaskContextImpl" -A 15 echo -e "\nChecking task_id usage in TaskContext..." rg "task_id" --type cpp -A 2Length of output: 60672
| // Parse task id | ||
| boost::uuids::string_generator const gen; | ||
| boost::uuids::uuid const task_id = gen(task_id_string); | ||
|
|
There was a problem hiding this comment.
Add explicit error handling for UUID parsing.
The UUID parsing could throw an exception if the task_id_string is not a valid UUID format. Consider adding explicit error handling.
// Parse task id
boost::uuids::string_generator const gen;
- boost::uuids::uuid const task_id = gen(task_id_string);
+ boost::uuids::uuid task_id;
+ try {
+ task_id = gen(task_id_string);
+ } catch (std::runtime_error const& e) {
+ spdlog::error("Invalid task ID format: {}", e.what());
+ return cCmdArgParseErr;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Parse task id | |
| boost::uuids::string_generator const gen; | |
| boost::uuids::uuid const task_id = gen(task_id_string); | |
| // Parse task id | |
| boost::uuids::string_generator const gen; | |
| boost::uuids::uuid task_id; | |
| try { | |
| task_id = gen(task_id_string); | |
| } catch (std::runtime_error const& e) { | |
| spdlog::error("Invalid task ID format: {}", e.what()); | |
| return cCmdArgParseErr; | |
| } |
1 participant
Description
As title.
Validation performed
Summary by CodeRabbit
Release Notes
New Features
TaskContextwith enhanced task management capabilities.FunctionNameManager.set_task_runningfunctionality.task_id.Improvements
Bug Fixes
These changes collectively enhance the Spider framework's robustness, type safety, and task management capabilities.