feat: Add storage interface - #14
Conversation
Added interface for metadata and storage interface. Added simple Task and Data class for interface to work. Add boost header for uuid and abseil for flat_hash_map.
WalkthroughThis pull request introduces several changes across multiple files, including the addition of a new entry to the Changes
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: 34
🧹 Outside diff range and nitpick comments (20)
.gitmodules (1)
7-9: Consider documenting the dependency's purpose.
Since abseil-cpp is a comprehensive utility library, it would be helpful to document which specific components are being utilized for the storage interface implementation.
Consider adding a comment in the README.md or documentation to explain:
- Why abseil-cpp was chosen
- Which specific components are being used
- Any version/compatibility requirements
Would you like me to help draft this documentation?
tools/scripts/linux/install-lib.sh (1)
Line range hint 1-15: Consider enhancing script robustness
The script could benefit from these improvements:
- Add version validation
- Include installation verification
- Use more robust path handling
Here's a suggested improvement:
#!/usr/bin/env bash
# Exit on any error
set -e
# Error on undefined variable
set -u
-script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
+# More robust path handling
+script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd -P )"
lib_install_scripts_dir=$script_dir/..
+# Function to verify installation
+verify_installation() {
+ local lib_name="$1"
+ echo "Verifying $lib_name installation..."
+}
+
"$lib_install_scripts_dir"/lib_install/fmtlib.sh 11.0.2
+verify_installation "fmtlib"
"$lib_install_scripts_dir"/lib_install/spdlog.sh 1.14.1
+verify_installation "spdlog"
"$lib_install_scripts_dir"/lib_install/mariadb-connector-c.sh 3.4.1
+verify_installation "mariadb-connector-c"
"$lib_install_scripts_dir"/lib_install/boost.sh 1.86.0
+verify_installation "boost"src/spider/core/Error.hpp (1)
1-29: Consider adopting a more structured error handling pattern.
The current error handling approach is simple and functional, but consider these architectural improvements:
- Consider implementing
std::error_codesupport for better integration with standard C++ error handling - Add static factory methods for common errors to ensure consistent error messages
- Consider adding error categories (similar to std::error_category) for different storage backends
Example of how this could evolve:
#include <system_error>
namespace spider::core {
enum class StorageErrType : std::uint8_t { ... };
class StorageErrorCategory : public std::error_category {
const char* name() const noexcept override;
std::string message(int ev) const override;
};
std::error_code make_error_code(StorageErrType e);
} // namespace spider::core
// Enable automatic conversion
namespace std {
template<>
struct is_error_code_enum<spider::core::StorageErrType> : true_type {};
}src/spider/CMakeLists.txt (1)
2-13: Consider improving comment style for consistency
The comment "spider core source files" could be more descriptive and follow a consistent style. Consider using a more detailed description that explains the purpose of these core files.
- "spider core source files"
+ "Core source files for Spider's storage interface and task management system"src/spider/core/Data.hpp (1)
1-9: Add class documentation for thread safety guarantees
Consider adding class-level documentation to explicitly state thread safety guarantees, as this class might be used in concurrent storage operations.
Add before the class declaration:
+/**
+ * Class representing a data entity with optional key and UUID.
+ * Thread safety: This class is immutable after construction and is thread-safe for concurrent reads.
+ */
class Data {tools/scripts/lib_install/boost.sh (1)
46-46: Add newline at end of file
Add a newline at the end of the file to comply with POSIX standards.
src/spider/storage/MetadataStorage.hpp (3)
13-20: Consider adding an explicit default constructor.
While the Rule of Five is properly implemented, consider adding:
MetadataStorage() = default;This makes the interface more complete and explicit about its construction behaviour.
24-29: Add documentation for task graph operations.
Consider adding Doxygen-style documentation for these methods, especially for:
- Success/failure conditions
- Ownership semantics of TaskGraph objects
- Concurrent access behaviour
40-41: Consider adding heartbeat configuration parameters.
The heartbeat mechanism might benefit from configurable timeouts and intervals. Consider adding methods to configure these parameters or accepting them through a configuration object.
CMakeLists.txt (1)
120-128: Important licensing consideration for MariaDBClient
The warning about GPL licensing restrictions for static linking is crucial. Consider documenting this limitation in the project's README or documentation to ensure future maintainers are aware of this constraint.
src/spider/storage/MysqlStorage.cpp (1)
1-131: Consider production readiness improvements.
Several important aspects should be addressed before production deployment:
- Add comprehensive error handling and logging
- Implement connection pooling for better scalability
- Add monitoring capabilities (e.g., query performance, connection status)
- Consider implementing retry mechanisms for transient failures
Would you like assistance in implementing any of these improvements?
🧰 Tools
🪛 cppcheck
[performance] 19-19: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 24-24: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 101-101: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 27-27: Function parameter 'value' should be passed by const reference.
(passedByValue)
[performance] 53-53: Function parameter 'value' should be passed by const reference.
(passedByValue)
src/spider/core/TaskGraph.hpp (6)
23-27: Use const references in range-based for loops to avoid unnecessary copies
In the loop over parents, iterating by const reference avoids unnecessary copies of boost::uuids::uuid objects.
Apply this diff to improve efficiency:
-for (boost::uuids::uuid const parent_id : parents) {
+for (boost::uuids::uuid const& parent_id : parents) {
if (!m_tasks.contains(parent_id)) {
return false;
}
}33-35: Use const references in range-based for loops to avoid unnecessary copies
Similarly, updating this loop enhances performance by preventing unnecessary copies.
Apply this diff:
-for (boost::uuids::uuid const parent_id : parents) {
+for (boost::uuids::uuid const& parent_id : parents) {
m_dependencies.emplace_back(parent_id, task_id);
}49-53: Iterate over dependencies by const reference to improve efficiency
Using const references in the loop over m_dependencies prevents copying of the pairs, enhancing performance.
Apply this diff:
-for (std::pair<boost::uuids::uuid, boost::uuids::uuid> const dep : m_dependencies) {
+for (std::pair<boost::uuids::uuid, boost::uuids::uuid> const& dep : m_dependencies) {
if (dep.first == id) {
children.emplace_back(dep.second);
}
}60-64: Iterate over dependencies by const reference to improve efficiency
As with the previous loop, using const references here enhances performance.
Apply this diff:
-for (std::pair<boost::uuids::uuid, boost::uuids::uuid> const dep : m_dependencies) {
+for (std::pair<boost::uuids::uuid, boost::uuids::uuid> const& dep : m_dependencies) {
if (dep.second == id) {
parents.emplace_back(dep.first);
}
}40-43: Optimize task retrieval by using find instead of contains and at
To avoid redundant hash map lookups, consider using find to retrieve the task directly.
Apply this diff:
-if (m_tasks.contains(id)) {
- return m_tasks.at(id);
-}
-return std::nullopt;
+auto it = m_tasks.find(id);
+if (it != m_tasks.end()) {
+ return it->second;
+}
+return std::nullopt;18-18: Introduce a type alias for dependency pairs to enhance readability
Defining a type alias for task dependencies improves code readability and maintainability.
Apply this change:
+#include <utility>
+
+using TaskDependency = std::pair<boost::uuids::uuid, boost::uuids::uuid>;
-class TaskGraph {
-private:
- absl::flat_hash_map<boost::uuids::uuid, Task> m_tasks;
- std::vector<std::pair<boost::uuids::uuid, boost::uuids::uuid>> m_dependencies;
+class TaskGraph {
+private:
+ absl::flat_hash_map<boost::uuids::uuid, Task> m_tasks;
+ std::vector<TaskDependency> m_dependencies;Remember to update other occurrences of std::pair<boost::uuids::uuid, boost::uuids::uuid> accordingly.
src/spider/storage/MysqlStorage.hpp (2)
55-58: Ensure consistent method declaration formatting
The formatting of method declarations in lines 55-62 is inconsistent with the rest of the code, which may affect readability. Consider adjusting the formatting to keep method declarations on a single line where possible.
Apply this diff to improve consistency:
-auto
-add_task_reference(boost::uuids::uuid id, boost::uuids::uuid task_id) -> StorageErr override;
+auto add_task_reference(boost::uuids::uuid id, boost::uuids::uuid task_id) -> StorageErr override;
-auto
-remove_task_reference(boost::uuids::uuid id, boost::uuids::uuid task_id) -> StorageErr override;
+auto remove_task_reference(boost::uuids::uuid id, boost::uuids::uuid task_id) -> StorageErr override;
-auto add_driver_reference(boost::uuids::uuid id, boost::uuids::uuid driver_id)
- -> StorageErr override;
+auto add_driver_reference(boost::uuids::uuid id, boost::uuids::uuid driver_id) -> StorageErr override;
-auto remove_driver_reference(boost::uuids::uuid id, boost::uuids::uuid driver_id)
- -> StorageErr override;
+auto remove_driver_reference(boost::uuids::uuid id, boost::uuids::uuid driver_id) -> StorageErr override;Also applies to: 59-62
16-63: Add documentation comments to public methods
Consider adding documentation comments to the public methods of MySqlMetadataStorage and MySqlDataStorage classes to enhance code readability and maintainability.
src/spider/core/Task.hpp (1)
75-75: Correct the enumeration value to 'Succeeded' for grammatical accuracy
In the TaskState enum, the state Succeed should be changed to Succeeded to maintain consistent past tense with other states like Failed.
Apply this diff:
Running,
- Succeed,
+ Succeeded,
Failed,📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- .gitignore (1 hunks)
- .gitmodules (1 hunks)
- CMakeLists.txt (2 hunks)
- src/spider/CMakeLists.txt (2 hunks)
- src/spider/core/Data.hpp (1 hunks)
- src/spider/core/Error.hpp (1 hunks)
- src/spider/core/Task.hpp (1 hunks)
- src/spider/core/TaskGraph.hpp (1 hunks)
- src/spider/storage/DataStorage.hpp (1 hunks)
- src/spider/storage/MetadataStorage.hpp (1 hunks)
- src/spider/storage/MysqlStorage.cpp (1 hunks)
- src/spider/storage/MysqlStorage.hpp (1 hunks)
- submodules/abseil-cpp (1 hunks)
- tools/scripts/lib_install/boost.sh (1 hunks)
- tools/scripts/linux/install-lib.sh (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- submodules/abseil-cpp
🧰 Additional context used
🪛 cppcheck
src/spider/storage/MysqlStorage.cpp
[performance] 19-19: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 24-24: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 101-101: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 27-27: Function parameter 'value' should be passed by const reference.
(passedByValue)
[performance] 53-53: Function parameter 'value' should be passed by const reference.
(passedByValue)
🔇 Additional comments (7)
tools/scripts/linux/install-lib.sh (1)
15-15: Verify Boost version compatibility
The script installs Boost 1.86.0, but the CMakeLists.txt requires version 1.74. While newer versions are typically backwards compatible, it would be prudent to document this decision or align the versions.
src/spider/core/Error.hpp (1)
1-7: LGTM! Well-structured header organization.
The include guards follow proper naming conventions, and the included headers are minimal and appropriate for the functionality needed.
src/spider/CMakeLists.txt (1)
30-34: Verify dependency configuration
The new library dependencies look good, but we should verify that these libraries are properly configured in the parent CMakeLists.txt file.
✅ Verification successful
Let me check the configuration of abseil-cpp to ensure it's properly set up.
Dependencies are properly configured
The dependency configuration is correct:
- Boost is properly configured with minimum version 1.74 and REQUIRED flag
- abseil-cpp is included as a submodule and added via
add_subdirectorywith EXCLUDE_FROM_ALL flag - Both dependencies are configured in the root CMakeLists.txt file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper dependency configuration in parent CMakeLists.txt files
# Test 1: Check for find_package commands for Boost and abseil
echo "Checking for dependency configuration..."
rg -l "find_package.*Boost" $(fd CMakeLists.txt)
rg -l "add_subdirectory.*abseil-cpp" $(fd CMakeLists.txt)
# Test 2: Check if minimum versions are specified for Boost
rg "find_package.*Boost.*REQUIRED" $(fd CMakeLists.txt)
Length of output: 524
Script:
#!/bin/bash
# Check abseil-cpp configuration details
echo "Checking abseil-cpp configuration..."
rg "add_subdirectory.*abseil-cpp" -A 5 $(fd CMakeLists.txt)
# Check if there are any version constraints for abseil-cpp
echo "Checking abseil-cpp version constraints..."
rg "abseil-cpp.*version|VERSION" $(fd CMakeLists.txt)
# Check if abseil-cpp is fetched or included as a dependency
echo "Checking how abseil-cpp is included..."
rg "FetchContent.*abseil-cpp|include.*abseil-cpp" $(fd CMakeLists.txt)
Length of output: 1216
src/spider/core/Data.hpp (1)
27-31: LGTM! Getter methods are well-designed
The getter methods are appropriately designed, returning by value which is suitable for these types. The consistent use of trailing return type syntax is good for readability.
src/spider/storage/MetadataStorage.hpp (1)
1-11: LGTM! Verify std::vector usage.
The header organization follows best practices with system headers separated from project headers. All includes appear necessary, but let's verify the std::vector usage.
✅ Verification successful
The std::vector inclusion is necessary and properly used
The std::vector is extensively used throughout the codebase:
- In TaskGraph.hpp for managing dependencies and parent-child relationships
- In Task.hpp for storing inputs and outputs
- In MetadataStorage.hpp interface methods for handling collections of tasks and task graphs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify std::vector usage in method signatures
rg -l "std::vector" "src/spider/core/{Task,TaskGraph}.hpp"
Length of output: 135
Script:
#!/bin/bash
# Let's first find the correct paths for Task.hpp and TaskGraph.hpp
fd "Task\.hpp|TaskGraph\.hpp" --type f
# Then check for std::vector usage in these files
rg "std::vector" --type cpp -A 2
Length of output: 5981
CMakeLists.txt (2)
80-89: LGTM: Boost configuration is well-structured
The Boost library configuration follows CMake best practices with proper version checking and error handling. The static/shared library handling aligns with the project's configuration.
131-133: Verify abseil-cpp integration impact
The integration looks correct, but since abseil-cpp is a significant dependency:
Let's verify the integration:
✅ Verification successful
Abseil-cpp integration is properly configured and used
The verification confirms that:
- The abseil-cpp submodule is properly initialized
- The library is used appropriately in the codebase:
absl::flat_hash_mapis used in TaskGraph.hpp for efficient task storage- The dependency is correctly linked in src/spider/CMakeLists.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify abseil-cpp submodule and its usage
# Check if the submodule is properly initialized
git submodule status submodules/abseil-cpp
# Look for abseil usage to ensure it's needed
rg -g '!{build,submodules}/*' -l 'absl::'
Length of output: 206
Script:
#!/bin/bash
# Check actual usage of abseil in the codebase
rg -g '!{build,submodules}/*' 'absl::' -A 3
# Check CMake configuration for abseil usage
cat src/spider/CMakeLists.txt
Length of output: 1629
| [submodule "submodules/abseil-cpp"] | ||
| path = submodules/abseil-cpp | ||
| url = https://github.com/abseil/abseil-cpp.git |
There was a problem hiding this comment.
💡 Codebase verification
Submodule needs initialization
The submodule directory is empty, indicating that it hasn't been initialized. Please run:
git submodule update --init --recursive
🔗 Analysis chain
LGTM! Verify submodule initialization.
The addition of abseil-cpp as a submodule follows the project's conventions.
Let's verify the submodule initialization:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the abseil-cpp submodule is properly initialized
# Test: Check if the submodule directory exists and is not empty
if [ -d "submodules/abseil-cpp" ] && [ "$(ls -A submodules/abseil-cpp)" ]; then
echo "Submodule is properly initialized"
else
echo "Warning: Submodule directory is missing or empty"
fi
# Test: Verify the submodule commit
git submodule status submodules/abseil-cpp
Length of output: 294
| enum class StorageErrType : std::uint8_t { | ||
| ConnectionErr, | ||
| DbNotFound, | ||
| KeyNotFoundErr, | ||
| DuplicateKeyErr, | ||
| ConstraintViolationErr, | ||
| Success | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the error type enumeration.
While the enum class implementation is good for type safety, consider these improvements:
- Move
Successto be the first value (0) as it's the default/normal case - Add common error types like
InvalidArgumentErr - Add documentation for each error type
Here's a suggested improvement:
enum class StorageErrType : std::uint8_t {
+ // Operation completed successfully
+ Success = 0,
+ // Failed to establish or maintain database connection
ConnectionErr,
+ // Specified database does not exist
DbNotFound,
+ // Requested key was not found in storage
KeyNotFoundErr,
+ // Attempted to insert a key that already exists
DuplicateKeyErr,
+ // Operation would violate a database constraint
ConstraintViolationErr,
- Success
+ // Invalid arguments provided to storage operation
+ InvalidArgumentErr
};📝 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.
| enum class StorageErrType : std::uint8_t { | |
| ConnectionErr, | |
| DbNotFound, | |
| KeyNotFoundErr, | |
| DuplicateKeyErr, | |
| ConstraintViolationErr, | |
| Success | |
| }; | |
| enum class StorageErrType : std::uint8_t { | |
| // Operation completed successfully | |
| Success = 0, | |
| // Failed to establish or maintain database connection | |
| ConnectionErr, | |
| // Specified database does not exist | |
| DbNotFound, | |
| // Requested key was not found in storage | |
| KeyNotFoundErr, | |
| // Attempted to insert a key that already exists | |
| DuplicateKeyErr, | |
| // Operation would violate a database constraint | |
| ConstraintViolationErr, | |
| // Invalid arguments provided to storage operation | |
| InvalidArgumentErr | |
| }; |
| struct StorageErr { | ||
| StorageErrType type; | ||
| std::string description; | ||
|
|
||
| StorageErr(StorageErrType type, std::string description) | ||
| : type(type), | ||
| description(std::move(description)) {} | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the StorageErr structure with additional functionality.
The structure could benefit from additional features to make it more robust and easier to use:
Consider these improvements:
struct StorageErr {
StorageErrType type;
std::string description;
+ // Default constructor for Success case
+ StorageErr() : type(StorageErrType::Success), description() {}
+
+ // Main constructor
StorageErr(StorageErrType type, std::string description)
: type(type),
description(std::move(description)) {}
+
+ // Convenience method to check success
+ [[nodiscard]] bool is_success() const {
+ return type == StorageErrType::Success;
+ }
+
+ // Enable comparison
+ bool operator==(const StorageErr& other) const {
+ return type == other.type;
+ }
};This adds:
- A default constructor for the success case
- A convenience method to check success
- Comparison operator for easier testing
- The [[nodiscard]] attribute to ensure error checking isn't accidentally ignored
Committable suggestion was skipped due to low confidence.
| target_sources( | ||
| spider_worker | ||
| PRIVATE | ||
| ${SPIDER_CORE_SOURCES} | ||
| ${SPIDER_WORKER_SOURCES} | ||
| ) |
There was a problem hiding this comment.
Remove duplicate target_sources command
This target_sources command is an exact duplicate of the one above. Having duplicate commands doesn't provide any benefit and could lead to maintenance issues.
-target_sources(
- spider_worker
- PRIVATE
- ${SPIDER_CORE_SOURCES}
- ${SPIDER_WORKER_SOURCES}
-)📝 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.
| target_sources( | |
| spider_worker | |
| PRIVATE | |
| ${SPIDER_CORE_SOURCES} | |
| ${SPIDER_WORKER_SOURCES} | |
| ) |
| class Data { | ||
| private: | ||
| boost::uuids::uuid m_id; | ||
| std::optional<std::string> m_key; | ||
| std::string m_value; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance const correctness and exception specifications
The class design is good with proper encapsulation. Consider enhancing it with const correctness and explicit exception specifications.
- auto get_id() -> boost::uuids::uuid { return m_id; }
+ auto get_id() const noexcept -> boost::uuids::uuid { return m_id; }
- auto get_key() -> std::optional<std::string> { return m_key; }
+ auto get_key() const noexcept -> std::optional<std::string> { return m_key; }
- auto get_value() -> std::string { return m_value; }
+ auto get_value() const noexcept -> std::string { return m_value; }Committable suggestion was skipped due to low confidence.
| return m_data_id; | ||
| } | ||
|
|
||
| [[nodiscard]] auto get_type() const -> std::string { return m_type; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return const reference from getter to avoid unnecessary copying
The get_type() function returns a std::string by value, which may cause unnecessary copying. Consider returning a const std::string& to improve performance.
Apply this diff:
- [[nodiscard]] auto get_type() const -> std::string { return m_type; }
+ [[nodiscard]] auto get_type() const -> const std::string& { return m_type; }📝 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.
| [[nodiscard]] auto get_type() const -> std::string { return m_type; } | |
| [[nodiscard]] auto get_type() const -> const std::string& { return m_type; } |
| std::string m_type; | ||
|
|
||
| public: | ||
| TaskOutput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize constructor by moving parameters to avoid unnecessary copying
In the TaskOutput constructor, consider using std::move for value to avoid unnecessary copying.
Apply this diff:
- TaskOutput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {}
+ TaskOutput(std::string value, std::string type) : m_value(std::move(value)), m_type(std::move(type)) {}📝 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.
| TaskOutput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {} | |
| TaskOutput(std::string value, std::string type) : m_value(std::move(value)), m_type(std::move(type)) {} |
| TaskInput(boost::uuids::uuid output_task_id, std::uint8_t position, std::string type) | ||
| : m_task_output({output_task_id, position}), | ||
| m_type(std::move(type)) {}; | ||
| TaskInput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize constructor by moving parameters to avoid unnecessary copying
In the TaskInput constructor, consider using std::move for value to avoid unnecessary copying.
Apply this diff:
- TaskInput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {};
+ TaskInput(std::string value, std::string type) : m_value(std::move(value)), m_type(std::move(type)) {};📝 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.
| TaskInput(std::string value, std::string type) : m_value(value), m_type(std::move(type)) {}; | |
| TaskInput(std::string value, std::string type) : m_value(std::move(value)), m_type(std::move(type)) {}; |
|
|
||
| [[nodiscard]] auto get_id() const -> boost::uuids::uuid { return m_id; } | ||
|
|
||
| [[nodiscard]] auto get_function_name() const -> std::string { return m_function_name; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return const reference from getter to avoid unnecessary copying
The get_function_name() function returns a std::string by value, which may cause unnecessary copying. Consider returning a const std::string& to improve performance.
Apply this diff:
- [[nodiscard]] auto get_function_name() const -> std::string { return m_function_name; }
+ [[nodiscard]] auto get_function_name() const -> const std::string& { return m_function_name; }📝 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.
| [[nodiscard]] auto get_function_name() const -> std::string { return m_function_name; } | |
| [[nodiscard]] auto get_function_name() const -> const std::string& { return m_function_name; } |
|
|
||
| [[nodiscard]] auto get_num_outputs() const -> size_t { return m_outputs.size(); } | ||
|
|
||
| [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { return m_inputs[index]; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return const reference from getter to avoid unnecessary copying
The get_input() function returns a TaskInput by value, which may result in unnecessary copying. Consider returning a const TaskInput& instead.
Apply this diff:
- [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { return m_inputs[index]; }
+ [[nodiscard]] auto get_input(uint64_t index) const -> const TaskInput& { return m_inputs[index]; }Similarly for get_output():
- [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput { return m_outputs[index]; }
+ [[nodiscard]] auto get_output(uint64_t index) const -> const TaskOutput& { return m_outputs[index]; }Committable suggestion was skipped due to low confidence.
sitaowang1998
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/spider/core/Task.hpp (2)
74-86: Add documentation for enum values
Consider adding documentation for each enum value to clarify:
- The conditions that trigger each TaskState
- The implications of each state
- The specific use cases for each TaskCreatorType
Example:
/// Represents the various states a task can be in during its lifecycle
enum class TaskState : std::uint8_t {
Pending, ///< Task is created but not yet ready for execution
Ready, ///< Task's dependencies are met and it's ready to run
// ... etc
};88-131: Add state transition management
The Task class lacks methods to manage state transitions. Consider adding:
- A method to update the task state with validation
- State transition validation logic
- Optional state change callbacks
Example implementation:
void set_state(TaskState new_state) {
// Validate state transition
if (!is_valid_transition(m_state, new_state)) {
throw std::invalid_argument("Invalid state transition");
}
m_state = new_state;
}
private:
static bool is_valid_transition(TaskState from, TaskState to) {
// Define valid state transitions
if (from == TaskState::Pending && to == TaskState::Ready) return true;
if (from == TaskState::Ready && to == TaskState::Running) return true;
// ... etc
return false;
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/spider/CMakeLists.txt (2 hunks)
- src/spider/core/Data.hpp (1 hunks)
- src/spider/core/Error.hpp (1 hunks)
- src/spider/core/Task.hpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/spider/CMakeLists.txt
- src/spider/core/Data.hpp
- src/spider/core/Error.hpp
🔇 Additional comments (2)
src/spider/core/Task.hpp (2)
72-72: Document or implement TaskInstance class
The empty class declaration should either be documented to explain its future purpose or be implemented if it's currently needed.
Would you like me to help create a basic implementation or documentation for this class?
34-37:
Add bounds checking for tuple access in get_task_output()
The tuple access could be unsafe if the optional is empty. Consider adding validation or using std::tie for safer tuple handling.
Apply this diff:
[[nodiscard]] auto get_task_output(
) const -> std::optional<std::tuple<boost::uuids::uuid, std::uint8_t>> {
- return m_task_output;
+ if (!m_task_output) {
+ return std::nullopt;
+ }
+ return m_task_output;
}Likely invalid or redundant comment.
| std::vector<TaskInput> m_inputs; | ||
| std::vector<TaskOutput> m_outputs; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pre-allocating vectors if size is known
If the number of inputs/outputs is known at construction time, consider using reserve() to prevent reallocation.
Example:
// In constructor
m_inputs.reserve(expected_inputs);
m_outputs.reserve(expected_outputs);| [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { return m_inputs[index]; } | ||
|
|
||
| [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput { return m_outputs[index]; } |
There was a problem hiding this comment.
Add bounds checking for vector access
The get_input and get_output methods should validate the index before accessing the vectors to prevent out-of-bounds access.
Apply this diff:
- [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { return m_inputs[index]; }
+ [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput {
+ if (index >= m_inputs.size()) {
+ throw std::out_of_range("Input index out of bounds");
+ }
+ return m_inputs[index];
+ }
- [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput { return m_outputs[index]; }
+ [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput {
+ if (index >= m_outputs.size()) {
+ throw std::out_of_range("Output index out of bounds");
+ }
+ return m_outputs[index];
+ }📝 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.
| [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { return m_inputs[index]; } | |
| [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput { return m_outputs[index]; } | |
| [[nodiscard]] auto get_input(uint64_t index) const -> TaskInput { | |
| if (index >= m_inputs.size()) { | |
| throw std::out_of_range("Input index out of bounds"); | |
| } | |
| return m_inputs[index]; | |
| } | |
| [[nodiscard]] auto get_output(uint64_t index) const -> TaskOutput { | |
| if (index >= m_outputs.size()) { | |
| throw std::out_of_range("Output index out of bounds"); | |
| } | |
| return m_outputs[index]; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/spider/storage/MysqlStorage.cpp (1)
13-13: Improve parameter naming in method declarations.
Instead of commenting out parameter names, consider:
- Using meaningful parameter names in the declaration
- Adding [[nodiscard]] attribute to emphasize return value importance
- Using more descriptive names for UUID parameters
Example refactor:
[[nodiscard]] auto connect(std::string const& connection_url,
boost::uuids::uuid storage_id) -> StorageErr;
[[nodiscard]] auto get_task_graph(boost::uuids::uuid graph_id,
TaskGraph& out_task_graph) -> StorageErr;Also applies to: 27-28, 40-41, 53-54, 70-71, 75-76, 84-85, 107-108, 112-114, 118-120, 124-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/spider/storage/MysqlStorage.cpp (1 hunks)
🧰 Additional context used
🪛 cppcheck
src/spider/storage/MysqlStorage.cpp
[performance] 105-105: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (1)
src/spider/storage/MysqlStorage.cpp (1)
1-131: Verify schema creation and migration strategy.
The implementation will need a way to:
- Create required tables if they don't exist
- Handle schema migrations for future updates
- Validate schema version during initialization
Let's check for existing schema management:
🧰 Tools
🪛 cppcheck
[performance] 105-105: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
| auto MySqlMetadataStorage::connect(std::string /*url*/, boost::uuids::uuid /*id*/) -> StorageErr { | ||
| return StorageErr{}; | ||
| } |
There was a problem hiding this comment.
Implement consistent error handling across all methods.
The current implementation returns empty StorageErr objects. Consider implementing proper error handling:
- Define specific error types for different failure scenarios
- Return appropriate error messages
- Include error context (e.g., method name, operation type)
Example implementation pattern:
auto MySqlMetadataStorage::initialize() -> StorageErr {
if (nullptr == m_mysql) {
return StorageErr{StorageErrType::NotConnected, "Database connection not established"};
}
try {
// Implementation here
return StorageErr{StorageErrType::Success, ""};
} catch (const std::exception& e) {
return StorageErr{StorageErrType::InitializationFailed,
std::string("Failed to initialize storage: ") + e.what()};
}
}Also applies to: 19-21, 23-25, 27-30, 32-34, 36-38, 40-43, 45-47, 49-51, 53-56, 58-60, 62-64, 66-68, 70-73, 75-78, 80-82, 84-87
| auto MySqlDataStorage::connect(std::string /*url*/, boost::uuids::uuid /*id*/) -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| void MySqlDataStorage::close() {} | ||
|
|
||
| auto MySqlDataStorage::initialize() -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage::add_data(Data const& /*data*/) -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage::get_data(boost::uuids::uuid /*id*/, Data& /*data*/) -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage::add_task_reference(boost::uuids::uuid /*id*/, boost::uuids::uuid /*task_id*/) | ||
| -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage:: | ||
| remove_task_reference(boost::uuids::uuid /*id*/, boost::uuids::uuid /*task_id*/) | ||
| -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage:: | ||
| add_driver_reference(boost::uuids::uuid /*id*/, boost::uuids::uuid /*driver_id*/) | ||
| -> StorageErr { | ||
| return StorageErr{}; | ||
| } | ||
|
|
||
| auto MySqlDataStorage::remove_driver_reference( | ||
| boost::uuids::uuid /*id*/, | ||
| boost::uuids::uuid /*driver_id*/ | ||
| ) -> StorageErr { | ||
| return StorageErr{}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add destructor and logging support for MySqlDataStorage.
Consider the following improvements:
- Add a virtual destructor to ensure proper cleanup
- Implement logging for debugging and monitoring
- Add connection state validation
Example implementation:
class MySqlDataStorage {
public:
virtual ~MySqlDataStorage() {
close();
}
auto connect(std::string const& url, boost::uuids::uuid id) -> StorageErr {
SPDLOG_DEBUG("Connecting to MySQL storage with ID: {}",
boost::uuids::to_string(id));
// Implementation
}
// ... other methods
};🧰 Tools
🪛 cppcheck
[performance] 105-105: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
1 participant
Description
Add interface for metadata and data storage.
Validation performed
Summary by CodeRabbit
New Features
abseil-cppas a submodule for enhanced functionality.Bug Fixes
Documentation
Chores
.gitignoreto streamline project management.