feat: Add scheduler policy and data stroage garbage collection - #34
Conversation
WalkthroughThe pull request introduces several modifications to the Spider project, primarily affecting the build configuration and class definitions. Key changes include updates to the Changes
Possibly related PRs
Suggested reviewers
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: 6
🧹 Outside diff range and nitpick comments (8)
tests/storage/test-DataStorage.cpp (1)
77-81: Remove unnecessary blank lines for code clarityThere are multiple blank lines between lines 77 and 81. Removing extra blank lines improves code readability.
Apply this diff to condense the code:
TEMPLATE_LIST_TEST_CASE( "Add and get task key value data", "[storage]", spider::test::StorageTypeList -) +) {src/spider/storage/MysqlStorage.cpp (3)
Line range hint
1208-1235: Refactor to eliminate code duplication betweenadd_driver_dataandadd_task_data.The methods
add_driver_dataandadd_task_datashare significant overlapping code. Consider extracting the common functionality into a private helper method or utility function to enhance maintainability and reduce redundancy.Also applies to: 1247-1279
1286-1299: Avoid unnecessary rollback on read-only operations inget_data.The
get_datamethod callsm_conn->rollback();when no data is found. SinceSELECTstatements are read-only, rolling back the transaction is unnecessary and may impact performance. Recommend removing the rollback in this case.
1489-1517: Avoid unnecessary rollback on read-only operations inget_client_kv_dataandget_task_kv_data.In the methods
get_client_kv_dataandget_task_kv_data, callingm_conn->rollback();when no results are found is unnecessary for read-onlySELECTqueries. Removing these rollbacks can improve performance without affecting functionality.Also applies to: 1524-1552
src/spider/utils/TimedCache.hpp (2)
12-12: Consider making threshold configurable at compile timeThe default threshold of 5 seconds is hardcoded. Consider making it a template parameter for more flexibility.
-constexpr unsigned cDefaultThreshold = 5; +template<unsigned DefaultThreshold = 5> class TimedCache {
40-45: Consider automatic cleanup and size limitsThe
cleanupmethod:
- Must be called explicitly, which could lead to unbounded memory growth
- No maximum size limit for the cache
Consider:
- Adding a size-based eviction policy
- Implementing automatic cleanup in
putoperations- Adding a max size parameter to constructor
tests/scheduler/test-SchedulerPolicy.cpp (2)
129-132: Consider extracting common test setup logic.The setup code for both hard and soft locality tests follows the same pattern. Consider creating a helper function to reduce code duplication.
Here's a suggested refactor:
+ namespace { + struct TestSetup { + boost::uuids::uuid client_id; + boost::uuids::uuid job_id; + spider::core::Task task; + spider::core::Data data; + }; + + TestSetup create_test_setup( + std::shared_ptr<spider::core::MetadataStorage> const& metadata_store, + std::shared_ptr<spider::core::DataStorage> const& data_store, + bool hard_locality = false) { + boost::uuids::random_generator gen; + TestSetup setup; + setup.client_id = gen(); + setup.job_id = gen(); + setup.task = spider::core::Task{"task"}; + setup.data = spider::core::Data{}; + setup.data.set_hard_locality(hard_locality); + setup.data.set_locality({"127.0.0.1"}); + + metadata_store->add_driver(setup.client_id, "127.0.0.1"); + data_store->add_driver_data(setup.client_id, setup.data); + return setup; + } + }Also applies to: 137-138, 142-142
Line range hint
86-142: Verify test coverage for edge cases.While the happy path testing is thorough, consider adding test cases for:
- Driver registration failure scenarios
- Data addition with non-existent drivers
- Multiple drivers with the same data
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
src/spider/CMakeLists.txt(1 hunks)src/spider/core/Data.hpp(2 hunks)src/spider/core/KeyValueData.hpp(1 hunks)src/spider/scheduler/FifoPolicy.cpp(2 hunks)src/spider/scheduler/FifoPolicy.hpp(2 hunks)src/spider/scheduler/SchedulerPolicy.hpp(1 hunks)src/spider/storage/DataStorage.hpp(3 hunks)src/spider/storage/MysqlStorage.cpp(7 hunks)src/spider/storage/MysqlStorage.hpp(3 hunks)src/spider/utils/TimedCache.hpp(1 hunks)tests/scheduler/test-SchedulerPolicy.cpp(2 hunks)tests/storage/test-DataStorage.cpp(4 hunks)tests/utils/CoreDataUtils.hpp(0 hunks)tests/worker/test-FunctionManager.cpp(0 hunks)
💤 Files with no reviewable changes (2)
- tests/utils/CoreDataUtils.hpp
- tests/worker/test-FunctionManager.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/scheduler/FifoPolicy.cpp
[performance] 88-88: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
tests/storage/test-DataStorage.cpp
[error] 48-48: syntax error
(syntaxError)
🔇 Additional comments (16)
src/spider/storage/DataStorage.hpp (4)
10-10: Include KeyValueData.hpp for key-value data operations
Including ../core/KeyValueData.hpp is appropriate and necessary for handling key-value data within the DataStorage class.
26-27: Consistent method names for adding data
The introduction of add_driver_data and add_task_data methods enhances clarity by specifying the context of the data being added. This improves the interface's expressiveness and maintains consistency.
38-38: Add remove_dangling_data method for data integrity
Implementing remove_dangling_data is a valuable addition to manage data integrity and prevent orphaned data within the storage system.
40-51: Implement key-value data management methods
The new methods for handling client and task key-value data (add_client_kv_data, add_task_kv_data, get_client_kv_data, get_task_kv_data) are well-defined and align with the system's requirements for key-value data operations.
src/spider/storage/MysqlStorage.hpp (3)
16-16: Include KeyValueData.hpp for key-value operations
Including ../core/KeyValueData.hpp is necessary for the implementation of key-value data methods in MySqlDataStorage.
85-86: Implement data addition methods with context identifiers
The methods add_driver_data and add_task_data correctly override the base class methods and provide context-specific data storage functionality.
97-110: Add key-value data management methods
The addition of key-value data methods enhances the MySqlDataStorage class's capabilities and aligns with the interface defined in DataStorage.
src/spider/storage/MysqlStorage.cpp (1)
33-33: The inclusion of KeyValueData.hpp is appropriate.
Including the KeyValueData.hpp header file is necessary for the newly added key-value data functionality.
src/spider/core/KeyValueData.hpp (1)
1-30: Well-implemented KeyValueData class.
The KeyValueData class is correctly designed with proper use of std::move in the constructor and [[nodiscard]] attributes on the accessor methods. The code follows best practices in terms of encapsulation and coding standards.
src/spider/scheduler/FifoPolicy.hpp (2)
26-26: Simplified cleanup method improves interface clarity.
Changing the method from cleanup_job(boost::uuids::uuid job_id) to cleanup() simplifies the interface, making it more general and potentially allowing for broader cleanup operations.
29-30: Adoption of TimedCache enhances cache management.
Replacing absl::flat_hash_map with core::TimedCache for m_task_job_cache and m_job_time_cache introduces cache expiration and improved resource management. This change is beneficial for maintaining optimal performance and preventing stale data.
src/spider/scheduler/SchedulerPolicy.hpp (1)
30-30: Updated cleanup method aligns with interface changes.
The modification of the cleanup method in the base SchedulerPolicy class ensures consistency across derived classes and reflects the updated design that no longer requires a job-specific cleanup parameter.
src/spider/core/Data.hpp (2)
34-34: LGTM: Improved const correctness
Good improvement adding const to the hard parameter in set_hard_locality.
18-18: Verify impact of constructor signature change
The addition of const to the id parameter is a good improvement for const correctness. However, this change could affect existing code that uses this constructor.
✅ Verification successful
Let me search for any direct instantiations of the Data class to ensure we have a complete picture.
Let me try a different approach to find usages and inheritance.
Let me check the inheritance in MysqlStorage.hpp and look for any other potential usages.
The constructor signature change is safe to proceed
Based on the comprehensive analysis of the codebase:
- The
Dataconstructor is used directly only in test files and internal storage implementations - The
constaddition to the UUID parameter is a non-breaking change that improves const correctness - The only direct usage in production code is through the storage layer, which already handles the UUID as const
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor usage to verify impact
rg -A 2 "Data\(.*uuid.*\)" --type cpp
Length of output: 526
Script:
#!/bin/bash
# Search for direct Data instantiations
ast-grep --pattern 'Data($$$)'
# Also search for any inheritance from Data class
ast-grep --pattern 'class $_ : $$$Data$$$'
Length of output: 77
Script:
#!/bin/bash
# Search for Data class inheritance and usage patterns
rg -l "class.*:.*Data" --type cpp
rg -l "std::make.*<Data>" --type cpp
rg -l "new Data\(" --type cpp
# Also check for any test files that might use the constructor
fd -e cpp -e hpp test | xargs rg "Data\("
Length of output: 186
Script:
#!/bin/bash
# Check MysqlStorage.hpp content
cat src/spider/storage/MysqlStorage.hpp
# Search for any Data type usage in method parameters or return types
rg "Data[&\*]?" --type cpp -A 2
Length of output: 32203
src/spider/CMakeLists.txt (1)
13-13: Verify header dependencies
The new headers KeyValueData.hpp and TimedCache.hpp are added to core headers. Please ensure:
- All required dependencies are listed in the target_link_libraries
- Header include order is correct
Also applies to: 21-21
✅ Verification successful
Dependencies are properly configured
The verification shows that all required dependencies are correctly set up:
- KeyValueData.hpp uses boost::uuid, which is covered by
Boost::headersin spider_core's target_link_libraries - TimedCache.hpp uses absl::flat_hash_map, which is explicitly linked via
absl::flat_hash_mapin spider_core's target_link_libraries - Both headers are properly listed in SPIDER_CORE_HEADERS and included in the spider_core target
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for missing dependencies in new headers
rg -l "^#include.*boost|^#include.*absl" core/KeyValueData.hpp utils/TimedCache.hpp
Length of output: 208
Script:
#!/bin/bash
# Find the actual paths of these header files
fd -e hpp -e h "KeyValueData|TimedCache"
# After finding the files, let's check their content and dependencies
Length of output: 107
Script:
#!/bin/bash
# Now check the actual content of these files for dependencies
echo "=== KeyValueData.hpp dependencies ==="
rg "^#include" src/spider/core/KeyValueData.hpp
echo -e "\n=== TimedCache.hpp dependencies ==="
rg "^#include" src/spider/utils/TimedCache.hpp
# Also check CMakeLists.txt for target_link_libraries
echo -e "\n=== CMakeLists.txt target_link_libraries ==="
rg "target_link_libraries" src/spider/CMakeLists.txt
Length of output: 1062
Script:
#!/bin/bash
# Let's check the complete CMakeLists.txt content to see all dependencies
cat src/spider/CMakeLists.txt
Length of output: 4106
tests/scheduler/test-SchedulerPolicy.cpp (1)
86-86: LGTM! The driver-data relationship setup looks good.
The changes properly establish the relationship between client, driver, and data before testing the scheduling behaviour. The sequence of operations is correct: adding the driver first, then the driver data, and finally the job.
Let's verify the driver-data relationship handling:
Also applies to: 92-93, 97-97
✅ Verification successful
Driver-data relationship is properly implemented and verified
After reviewing the test file content, I can confirm that the driver-data relationship is correctly handled in both the "Schedule hard locality" and "Schedule soft locality" test cases. The sequence follows the proper pattern:
- Driver registration with
add_driver(client_id, "127.0.0.1") - Data addition with
add_driver_data(client_id, data) - Job submission with the task using the data
The changes in lines 86, 92-93, and 97 maintain consistency with this pattern and properly test the scheduling behaviour with respect to data locality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent driver-data relationship handling across tests
# Look for patterns where driver registration precedes data addition
ast-grep --pattern 'add_driver($_, $_)' -A 5
Length of output: 46
Script:
#!/bin/bash
# Let's try a different approach to find driver-related test patterns
# First, let's see the content of the test file
cat tests/scheduler/test-SchedulerPolicy.cpp
Length of output: 6740
| auto FifoPolicy::cleanup() -> void { | ||
| m_task_job_cache.cleanup(); | ||
| m_task_job_cache.cleanup(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct cache cleanup in cleanup() method
There is a duplicate call to m_task_job_cache.cleanup();, and m_job_time_cache.cleanup(); is missing. Ensure that both caches are properly cleaned up.
Apply this diff to fix the cleanup logic:
auto FifoPolicy::cleanup() -> void {
m_task_job_cache.cleanup();
- m_task_job_cache.cleanup();
+ m_job_time_cache.cleanup();
}📝 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.
| auto FifoPolicy::cleanup() -> void { | |
| m_task_job_cache.cleanup(); | |
| m_task_job_cache.cleanup(); | |
| auto FifoPolicy::cleanup() -> void { | |
| m_task_job_cache.cleanup(); | |
| m_job_time_cache.cleanup(); |
| if (std::optional<std::chrono::system_clock::time_point> const optional_time; | ||
| optional_time.has_value()) | ||
| { | ||
| return optional_time.value(); |
There was a problem hiding this comment.
Initialize optional_time to prevent logical error
Similarly, optional_time is declared without initialization, making optional_time.has_value() always return false. This prevents the intended caching mechanism from functioning.
Apply this diff to initialize optional_time correctly:
- if (std::optional<std::chrono::system_clock::time_point> const optional_time;
+ if (std::optional<std::chrono::system_clock::time_point> const optional_time = m_job_time_cache.get(job_id);
optional_time.has_value())
{
return optional_time.value();
}📝 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.
| if (std::optional<std::chrono::system_clock::time_point> const optional_time; | |
| optional_time.has_value()) | |
| { | |
| return optional_time.value(); | |
| if (std::optional<std::chrono::system_clock::time_point> const optional_time = m_job_time_cache.get(job_id); | |
| optional_time.has_value()) | |
| { | |
| return optional_time.value(); |
| std::optional<boost::uuids::uuid> optional_job_id; | ||
| if (optional_job_id.has_value()) { | ||
| job_id = optional_job_id.value(); |
There was a problem hiding this comment.
Initialize optional_job_id to prevent logical error
The variable optional_job_id is declared but not initialized, resulting in optional_job_id.has_value() always returning false. This causes the code block that assigns job_id to be skipped.
Apply this diff to initialize optional_job_id correctly:
- std::optional<boost::uuids::uuid> optional_job_id;
+ std::optional<boost::uuids::uuid> optional_job_id = m_task_job_cache.get(task_id);
if (optional_job_id.has_value()) {
job_id = optional_job_id.value();
} else {Committable suggestion skipped: line range outside the PR's diff.
| == data_storage->get_data(data.get_id(), &result).type); | ||
| } | ||
|
|
||
| TEMPLATE_LIST_TEST_CASE( |
There was a problem hiding this comment.
Fix syntax error due to unmatched parentheses in macro
There is a syntax error at line 48, likely caused by missing closing parentheses in the TEMPLATE_LIST_TEST_CASE macro. Ensure all parentheses are properly matched.
Apply this diff to close the macro definition correctly:
TEMPLATE_LIST_TEST_CASE(
"Add and get driver key value data",
"[storage]",
spider::test::StorageTypeList
+)📝 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.
| TEMPLATE_LIST_TEST_CASE( | |
| TEMPLATE_LIST_TEST_CASE( | |
| "Add and get driver key value data", | |
| "[storage]", | |
| spider::test::StorageTypeList | |
| ) |
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 48-48: syntax error
(syntaxError)
|
|
||
| std::array<char const* const, 12> const cCreateStorage = { | ||
| char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` ( | ||
| `key` VARCHAR(64) NOT NULL, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using key as a column name to prevent SQL keyword conflicts.
The column name key is a reserved keyword in SQL and may cause conflicts or require special handling. Consider renaming it to kv_key or data_key to improve clarity and avoid potential issues.
Apply this diff to rename the column:
-char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` (
- `key` VARCHAR(64) NOT NULL,
+ `kv_key` VARCHAR(64) NOT NULL,
`value` VARCHAR(128) NOT NULL,
`client_id` BINARY(16) NOT NULL,
- PRIMARY KEY (`client_id`, `key`)
+ PRIMARY KEY (`client_id`, `kv_key`)
))";
-char const* const cCreateTaskKVDataTable = R"(CREATE TABLE IF NOT EXISTS `task_kv_data` (
- `key` VARCHAR(64) NOT NULL,
+ `kv_key` VARCHAR(64) NOT NULL,
`value` VARCHAR(128) NOT NULL,
`task_id` BINARY(16) NOT NULL,
- PRIMARY KEY (`task_id`, `key`)
+ PRIMARY KEY (`task_id`, `kv_key`)
))";Also applies to: 169-169
| auto put(Key const& key, Value const& value) { | ||
| auto iter = m_map.find(key); | ||
| if (iter == m_map.end()) { | ||
| m_map[key] = std::make_pair(std::chrono::steady_clock::now(), value); | ||
| } else { | ||
| iter->second.first = std::chrono::steady_clock::now(); | ||
| iter->second.second = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add return type and consider thread safety
The put method:
- Is missing a return type (should be
void) - Could have race conditions in a multi-threaded environment
- auto put(Key const& key, Value const& value) {
+ void put(Key const& key, Value const& value) {Consider adding mutex protection for thread safety:
private:
mutable std::mutex m_mutex;
// ... existing members ...
public:
void put(Key const& key, Value const& value) {
std::lock_guard<std::mutex> lock(m_mutex);
// ... existing implementation ...
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/spider/scheduler/FifoPolicy.cpp (1)
Line range hint
99-112: Consider combining metadata store queries for better performance.While the implementation is correct, we could optimize performance by combining the metadata store queries. Currently, we might hit the metadata store twice for the same job (once for ID and once for creation time).
Consider caching both pieces of information in a single query when either is missing.
Here's a suggested optimization:
if (optional_job_id.has_value()) { job_id = optional_job_id.value(); + if (auto const optional_time = m_job_time_cache.get(job_id); + optional_time.has_value()) { + return optional_time.value(); + } } else { if (false == metadata_store->get_task_job_id(task_id, &job_id).success()) { throw std::runtime_error(fmt::format( "Task with id {} not exists.", boost::uuids::to_string(task_id) )); } m_task_job_cache.put(task_id, job_id); - } - - std::optional<std::chrono::system_clock::time_point> const optional_time - = m_job_time_cache.get(job_id); - if (optional_time.has_value()) { - return optional_time.value(); } core::JobMetadata job_metadata;🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 88-88: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/storage/MysqlStorage.cpp (4)
1248-1275: Consider extracting common data insertion logicThe code is duplicated between
add_driver_dataandadd_task_data. Consider extracting the common data and locality insertion logic into a private helper method.Example refactor:
+private: + auto insert_data_and_locality(Data const& data) -> StorageErr { + std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( + "INSERT INTO `data` (`id`, `value`, `hard_locality`) VALUES(?, ?, ?)" + )); + sql::bytes id_bytes = uuid_get_bytes(data.get_id()); + statement->setBytes(1, &id_bytes); + statement->setString(2, data.get_value()); + statement->setBoolean(3, data.is_hard_locality()); + statement->executeUpdate(); + + for (std::string const& addr : data.get_locality()) { + std::unique_ptr<sql::PreparedStatement> locality_statement( + m_conn->prepareStatement("INSERT INTO `data_locality` (`id`, " + "`address`) VALUES (?, ?)") + ); + locality_statement->setBytes(1, &id_bytes); + locality_statement->setString(2, addr); + locality_statement->executeUpdate(); + } + return StorageErr{}; + }Then use it in both methods:
auto MySqlDataStorage::add_driver_data(boost::uuids::uuid const driver_id, Data const& data) -> StorageErr { try { - std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( - "INSERT INTO `data` (`id`, `value`, `hard_locality`) VALUES(?, ?, ?)" - )); - sql::bytes id_bytes = uuid_get_bytes(data.get_id()); - statement->setBytes(1, &id_bytes); - statement->setString(2, data.get_value()); - statement->setBoolean(3, data.is_hard_locality()); - statement->executeUpdate(); - - for (std::string const& addr : data.get_locality()) { - std::unique_ptr<sql::PreparedStatement> locality_statement( - m_conn->prepareStatement("INSERT INTO `data_locality` (`id`, " - "`address`) VALUES (?, ?)") - ); - locality_statement->setBytes(1, &id_bytes); - locality_statement->setString(2, addr); - locality_statement->executeUpdate(); - } + auto err = insert_data_and_locality(data); + if (err.has_error()) { + return err; + } + sql::bytes id_bytes = uuid_get_bytes(data.get_id()); std::unique_ptr<sql::PreparedStatement> driver_ref_statement(m_conn->prepareStatement( "INSERT INTO `data_ref_driver` (`id`, `driver_id`) VALUES(?, ?)" ));
1429-1441: Consider adding indexes to optimize the cleanup queryThe subqueries in the DELETE statement would benefit from indexes on the id columns of the reference tables.
Add these indexes to the table creation SQL:
char const* const cCreateDataRefDriverTable = R"(CREATE TABLE IF NOT EXISTS `data_ref_driver` ( `id` BINARY(16) NOT NULL, `driver_id` BINARY(16) NOT NULL, - KEY (`id`) USING BTREE, + KEY (`id`, `driver_id`) USING BTREE, KEY (`driver_id`) USING BTREE, CONSTRAINT `data_driver_ref_id` FOREIGN KEY (`id`) REFERENCES `data` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE, CONSTRAINT `data_ref_driver_id` FOREIGN KEY (`driver_id`) REFERENCES `drivers` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE ))"; char const* const cCreateDataRefTaskTable = R"(CREATE TABLE IF NOT EXISTS `data_ref_task` ( `id` BINARY(16) NOT NULL, `task_id` BINARY(16) NOT NULL, - KEY (`id`) USING BTREE, + KEY (`id`, `task_id`) USING BTREE, KEY (`task_id`) USING BTREE, CONSTRAINT `data_task_ref_id` FOREIGN KEY (`id`) REFERENCES `data` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE, CONSTRAINT `data_ref_task_id` FOREIGN KEY (`task_id`) REFERENCES `tasks` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE ))";
1443-1483: Extract common key-value data insertion logicBoth methods share similar insertion logic. Consider extracting the common code into a template method.
Example refactor:
+private: + template<typename T> + auto add_kv_data(T const& data, const char* table_name, const char* id_column) -> StorageErr { + try { + std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( + fmt::format("INSERT INTO `{}` (`kv_key`, `value`, `{}`) VALUES(?, ?, ?)", + table_name, id_column) + )); + statement->setString(1, data.get_key()); + statement->setString(2, data.get_value()); + sql::bytes id_bytes = uuid_get_bytes(data.get_id()); + statement->setBytes(3, &id_bytes); + statement->executeUpdate(); + } catch (sql::SQLException& e) { + m_conn->rollback(); + if (e.getErrorCode() == ErDupKey || e.getErrorCode() == ErDupEntry) { + return StorageErr{StorageErrType::DuplicateKeyErr, e.what()}; + } + return StorageErr{StorageErrType::OtherErr, e.what()}; + } + m_conn->commit(); + return StorageErr{}; + }Then simplify both methods:
auto MySqlDataStorage::add_client_kv_data(KeyValueData const& data) -> StorageErr { - try { - std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( - "INSERT INTO `client_kv_data` (`kv_key`, `value`, `client_id`) VALUES(?, ?, ?)" - )); - statement->setString(1, data.get_key()); - statement->setString(2, data.get_value()); - sql::bytes id_bytes = uuid_get_bytes(data.get_id()); - statement->setBytes(3, &id_bytes); - statement->executeUpdate(); - } catch (sql::SQLException& e) { - m_conn->rollback(); - if (e.getErrorCode() == ErDupKey || e.getErrorCode() == ErDupEntry) { - return StorageErr{StorageErrType::DuplicateKeyErr, e.what()}; - } - return StorageErr{StorageErrType::OtherErr, e.what()}; - } - m_conn->commit(); - return StorageErr{}; + return add_kv_data(data, "client_kv_data", "client_id"); }
1485-1518: Extract common key-value data retrieval logicThe method shares similar retrieval logic with
get_task_kv_data. Consider extracting the common code.Example refactor:
+private: + template<typename ID> + auto get_kv_data( + ID const& id, + std::string const& key, + std::string* value, + const char* table_name, + const char* id_column + ) -> StorageErr { + try { + std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( + fmt::format("SELECT `value` FROM `{}` WHERE `{}` = ? AND `kv_key` = ?", + table_name, id_column) + )); + sql::bytes id_bytes = uuid_get_bytes(id); + statement->setBytes(1, &id_bytes); + statement->setString(2, key); + std::unique_ptr<sql::ResultSet> res(statement->executeQuery()); + if (res->rowsCount() == 0) { + m_conn->rollback(); + return StorageErr{ + StorageErrType::KeyNotFoundErr, + fmt::format( + "no data for {} {} with key {}", + id_column, + boost::uuids::to_string(id), + key + ) + }; + } + res->next(); + *value = res->getString(1); + } catch (sql::SQLException& e) { + m_conn->rollback(); + return StorageErr{StorageErrType::OtherErr, e.what()}; + } + m_conn->commit(); + return StorageErr{}; + }Then simplify both methods:
auto MySqlDataStorage::get_client_kv_data( boost::uuids::uuid const& client_id, std::string const& key, std::string* value ) -> StorageErr { - try { - std::unique_ptr<sql::PreparedStatement> statement(m_conn->prepareStatement( - "SELECT `value` " - "FROM `client_kv_data` WHERE `client_id` = ? AND `kv_key` = ?" - )); - sql::bytes id_bytes = uuid_get_bytes(client_id); - statement->setBytes(1, &id_bytes); - statement->setString(2, key); - std::unique_ptr<sql::ResultSet> res(statement->executeQuery()); - if (res->rowsCount() == 0) { - m_conn->rollback(); - return StorageErr{ - StorageErrType::KeyNotFoundErr, - fmt::format( - "no data for client {} with key {}", - boost::uuids::to_string(client_id), - key - ) - }; - } - res->next(); - *value = res->getString(1); - } catch (sql::SQLException& e) { - m_conn->rollback(); - return StorageErr{StorageErrType::OtherErr, e.what()}; - } - m_conn->commit(); - return StorageErr{}; + return get_kv_data(client_id, key, value, "client_kv_data", "client_id"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/spider/scheduler/FifoPolicy.cpp(2 hunks)src/spider/storage/MysqlStorage.cpp(7 hunks)src/spider/utils/TimedCache.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spider/utils/TimedCache.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/scheduler/FifoPolicy.cpp
[performance] 88-88: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (4)
src/spider/scheduler/FifoPolicy.cpp (2)
85-96: LGTM! Proper cache usage and error handling.
The implementation correctly uses optional for cache retrieval and includes appropriate error handling with descriptive messages.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 88-88: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
120-122: LGTM! Fixed cache cleanup implementation.
The cleanup method now correctly handles both caches, addressing the issue from previous reviews where there was a duplicate call to m_task_job_cache.cleanup().
src/spider/storage/MysqlStorage.cpp (2)
Line range hint 1209-1236: LGTM!
The implementation is secure, properly handles transactions, and correctly manages data references.
1520-1553: LGTM!
The implementation is secure and follows good practices for database access.
| char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` ( | ||
| `kv_key` VARCHAR(64) NOT NULL, | ||
| `value` VARCHAR(128) NOT NULL, | ||
| `client_id` BINARY(16) NOT NULL, | ||
| PRIMARY KEY (`client_id`, `kv_key`) | ||
| ))"; |
There was a problem hiding this comment.
Add foreign key constraint for client_id
The client_kv_data table should have a foreign key constraint on client_id to maintain referential integrity with the clients table, similar to how task_kv_data has a constraint on task_id.
Apply this diff:
char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` (
`kv_key` VARCHAR(64) NOT NULL,
`value` VARCHAR(128) NOT NULL,
`client_id` BINARY(16) NOT NULL,
- PRIMARY KEY (`client_id`, `kv_key`)
+ PRIMARY KEY (`client_id`, `kv_key`),
+ CONSTRAINT `kv_data_client_id` FOREIGN KEY (`client_id`) REFERENCES `drivers` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE
))";📝 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.
| char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` ( | |
| `kv_key` VARCHAR(64) NOT NULL, | |
| `value` VARCHAR(128) NOT NULL, | |
| `client_id` BINARY(16) NOT NULL, | |
| PRIMARY KEY (`client_id`, `kv_key`) | |
| ))"; | |
| char const* const cCreateClientKVDataTable = R"(CREATE TABLE IF NOT EXISTS `client_kv_data` ( | |
| `kv_key` VARCHAR(64) NOT NULL, | |
| `value` VARCHAR(128) NOT NULL, | |
| `client_id` BINARY(16) NOT NULL, | |
| PRIMARY KEY (`client_id`, `kv_key`), | |
| CONSTRAINT `kv_data_client_id` FOREIGN KEY (`client_id`) REFERENCES `drivers` (`id`) ON UPDATE NO ACTION ON DELETE CASCADE | |
| ))"; |
1 participant
Description
As title.
Validation performed
Summary by CodeRabbit
New Features
KeyValueDataandTimedCacheclasses for enhanced data management.Bug Fixes
Tests