refactor: Add abstract factory pattern to storage - #78
Conversation
WalkthroughThe changes refactor Spider’s storage management by replacing MySQL-specific implementations with an abstract, factory-based design. New source files, headers, and updated test cases introduce a unified interface for storage connections using smart pointers and variants. Client, scheduler, worker, and core components now obtain storage resources via a common Changes
Sequence Diagram(s)sequenceDiagram
participant X as Client/Worker/Driver
participant SF as StorageFactory
participant SC as StorageConnection
X->>SF: Request storage connection
SF->>SC: Create connection (via MySqlConnection in factory)
alt Connection Successful
SC-->>SF: Return unique pointer
SF-->>X: Provide connection for storage operations
else Connection Failure
SF-->>X: Return error (StorageErr)
end
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (35)
src/spider/worker/task_executor.cpp (1)
129-131: Handle potential null returns from the factory
provide_metadata_storage()andprovide_data_storage()might return null pointers in error scenarios. Consider checking the returned pointers to avoid runtime crashes.src/spider/storage/mysql/MySqlConnection.cpp (1)
31-31: Prefer usingstd::make_unique
For clarity and exception safety, consider leveragingstd::make_unique:- return std::unique_ptr<StorageConnection>(new MySqlConnection{std::move(conn)}); + return std::make_unique<MySqlConnection>(std::move(conn));src/spider/worker/WorkerClient.hpp (2)
30-31: Constructor parameter expansion looks fine.
Ensure that callers properly supply a non-nullstorage_factoryto avoid runtime errors.
43-43: New member variable aligns well with the abstract factory pattern.
As a minor note, consider confirming thatm_storage_factorycannot be null, or detect and handle null usage.tests/storage/StorageTestHelper.hpp (1)
26-30: Consider making get_storage_url more genericThe
get_storage_urlfunction currently only works withMySqlStorageFactory, which is fine for now but might be limiting when adding more backends.Consider modifying this to support multiple storage backends by using a type trait or concept-based specialization approach instead of the
requiresconstraint. This would make it easier to add support for new storage backends in the future without modifying this function.src/spider/storage/StorageFactory.hpp (1)
16-22: Consider adding documentationWhile the function names are descriptive, adding documentation comments would help clarify the responsibility of each method, especially for new contributors. The abstract factory pattern implementation is solid, but documenting the contract each derived class must fulfill would be valuable.
public: + /** + * @brief Creates and provides a DataStorage instance. + * @return A unique pointer to a DataStorage implementation. + */ virtual auto provide_data_storage() -> std::unique_ptr<DataStorage> = 0; + /** + * @brief Creates and provides a MetadataStorage instance. + * @return A unique pointer to a MetadataStorage implementation. + */ virtual auto provide_metadata_storage() -> std::unique_ptr<MetadataStorage> = 0; + /** + * @brief Creates and provides a StorageConnection instance. + * @return A variant containing either a StorageConnection pointer or an error. + */ virtual auto provide_storage_connection( ) -> std::variant<std::unique_ptr<StorageConnection>, StorageErr> = 0; + /** + * @brief Creates and provides a JobSubmissionBatch instance using an existing connection. + * @param connection An existing StorageConnection to use. + * @return A unique pointer to a JobSubmissionBatch implementation. + */ virtual auto provide_job_submission_batch(StorageConnection&) -> std::unique_ptr<JobSubmissionBatch> = 0;tests/scheduler/test-SchedulerServer.cpp (1)
40-53: Consider extracting connection setup to a helper functionThe connection setup logic is somewhat complex and could be extracted into a helper function to improve readability and make the test focus more on what's being tested rather than how the connection is established.
+ namespace { + template <typename FactoryType> + std::tuple<std::unique_ptr<spider::core::StorageFactory>, + std::shared_ptr<spider::core::MetadataStorage>, + std::shared_ptr<spider::core::DataStorage>, + std::shared_ptr<spider::core::StorageConnection>> + setup_storage_components() { + std::unique_ptr<spider::core::StorageFactory> storage_factory + = spider::test::create_storage_factory<FactoryType>(); + std::shared_ptr<spider::core::MetadataStorage> metadata_store + = storage_factory->provide_metadata_storage(); + std::shared_ptr<spider::core::DataStorage> data_store = storage_factory->provide_data_storage(); + + std::variant<std::unique_ptr<spider::core::StorageConnection>, spider::core::StorageErr> + conn_result = storage_factory->provide_storage_connection(); + REQUIRE(std::holds_alternative<std::unique_ptr<spider::core::StorageConnection>>(conn_result)); + std::shared_ptr<spider::core::StorageConnection> conn + = std::move(std::get<std::unique_ptr<spider::core::StorageConnection>>(conn_result)); + + return {std::move(storage_factory), std::move(metadata_store), std::move(data_store), std::move(conn)}; + } + } + TEMPLATE_LIST_TEST_CASE( "Scheduler server test", "[scheduler][server][storage]", spider::test::StorageFactoryTypeList ) { - std::unique_ptr<spider::core::StorageFactory> storage_factory - = spider::test::create_storage_factory<TestType>(); - std::shared_ptr<spider::core::MetadataStorage> metadata_store - = storage_factory->provide_metadata_storage(); - std::shared_ptr<spider::core::DataStorage> data_store = storage_factory->provide_data_storage(); - - std::variant<std::unique_ptr<spider::core::StorageConnection>, spider::core::StorageErr> - conn_result = storage_factory->provide_storage_connection(); - REQUIRE(std::holds_alternative<std::unique_ptr<spider::core::StorageConnection>>(conn_result)); - std::shared_ptr<spider::core::StorageConnection> conn - = std::move(std::get<std::unique_ptr<spider::core::StorageConnection>>(conn_result)); + auto [storage_factory, metadata_store, data_store, conn] = setup_storage_components<TestType>();tests/scheduler/test-SchedulerPolicy.cpp (7)
34-38: Validate factory pointer contents.
Storing the result ofcreate_storage_factory<TestType>()in a shared pointer is valid. However, consider adding a check (beyond the unit test scope) to ensure the factory pointer is not null if this function can fail.
55-55: Check test coverage of add_job success path.
Verifying thatadd_jobreturns success is good. Consider negative tests for erroneous inputs or insufficient permissions if relevant.
63-63: Repeat the negative test for add_job.
Same consideration as above: your success check is thorough. A complementary failure scenario test can bolster coverage.
96-101: Factory creation repeated in tests.
While the pattern is consistent, consider extracting the setup logic (factory creation and storage initialization) into a test fixture if repeated across many tests to reduce boilerplate.
116-117: Driver data creation checks.
Ensuring driver data is successfully added is valuable. Include negative scenario tests (e.g., invalid driver ID) if possible.
145-149: Repeated setup logic.
Same suggestion as earlier: a reusable test fixture for retrieving metadata and data storage could enhance clarity and reduce duplication.
165-166: Driver insertion and data addition.
Again, verifying success is helpful. Consider adding coverage for insertion failures in parallel or under concurrency if relevant.tests/client/test-Driver.cpp (2)
6-6: Avoid duplicating the same header.
#include <catch2/catch_template_test_macros.hpp>is repeated on line 14. Consider removing one of them to comply with DRY principles.-#include <catch2/catch_template_test_macros.hpp> ... -#include "catch2/catch_template_test_macros.hpp" +#include <catch2/catch_template_test_macros.hpp>
14-14: Confirm necessity of double include.
The second inclusion of the same header is likely unnecessary. Simplify to a single inclusion.src/spider/worker/WorkerClient.cpp (3)
42-43: Safely storing shared pointers.
Storingm_metadata_storeandm_storage_factoryensures consistent usage. Confirm that no concurrency hazards arise from shared usage in multi-threaded contexts.
60-61: Encapsulate ownership of the storage connection.
Moving from unique_ptr to a local variable is acceptable. If a persistent connection is needed, consider storing it as a class member.
121-122: Repeat connection logic.
If multiple lookups are needed, examine whether reusing a single connection instance might reduce overhead.src/spider/storage/mysql/MySqlStorageFactory.cpp (3)
19-21: Consider usingstd::make_uniquefor consistency with modern C++.Replacing manual dynamic allocation with
std::make_unique<MySqlDataStorage>()can provide clearer code, reduce verbosity, and prevent potential memory leaks if exceptions are thrown before the raw pointer is assigned to thestd::unique_ptr.-auto MySqlStorageFactory::provide_data_storage() -> std::unique_ptr<DataStorage> { - return std::unique_ptr<DataStorage>(new MySqlDataStorage()); +auto MySqlStorageFactory::provide_data_storage() -> std::unique_ptr<DataStorage> { + return std::make_unique<MySqlDataStorage>(); }
23-25: Usestd::make_uniqueto streamline allocation.For consistency, consider using
std::make_unique<MySqlMetadataStorage>()rather than manualnew. This approach simplifies error handling, especially if exceptions are thrown.-auto MySqlStorageFactory::provide_metadata_storage() -> std::unique_ptr<MetadataStorage> { - return std::unique_ptr<MetadataStorage>(new MySqlMetadataStorage()); +auto MySqlStorageFactory::provide_metadata_storage() -> std::unique_ptr<MetadataStorage> { + return std::make_unique<MySqlMetadataStorage>(); }
37-40: Applystd::make_uniquefor factory consistency.When constructing
MySqlJobSubmissionBatch, preferstd::make_unique<MySqlJobSubmissionBatch>(connection). This maintains a uniform allocation pattern and simplifies catch-block scenarios.-auto MySqlStorageFactory::provide_job_submission_batch(StorageConnection& connection -) -> std::unique_ptr<JobSubmissionBatch> { - return std::unique_ptr<JobSubmissionBatch>(new MySqlJobSubmissionBatch(connection)); +auto MySqlStorageFactory::provide_job_submission_batch(StorageConnection& connection +) -> std::unique_ptr<JobSubmissionBatch> { + return std::make_unique<MySqlJobSubmissionBatch>(connection); }tests/worker/test-TaskExecutor.cpp (1)
158-163: Consider usingstd::unique_ptrin lieu ofstd::shared_ptrif single ownership suffices.If the data and metadata storage objects are not intended to be shared beyond this scope,
unique_ptrmight offer simpler ownership semantics and more clarity.src/spider/client/Driver.cpp (2)
53-54: Recreating connections in the heartbeat thread may be expensive.If not strictly required to open new connections each second, consider retaining a longer-lived connection to minimise overhead.
Also applies to: 58-58
68-70: Repeated re-connection in the second constructor’s heartbeat loop warrants review.If permissible, reuse an existing connection in each iteration to reduce potential connection overhead.
Also applies to: 71-73, 74-75, 79-79, 94-95, 99-99, 101-101
tests/worker/test-FunctionManager.cpp (2)
130-136: Repetitive factory usage.
The code here mirrors lines 71–77. A small utility function could eliminate duplication across tests.- std::unique_ptr<spider::core::StorageFactory> storage_factory - = spider::test::create_storage_factory<TestType>(); - std::unique_ptr<spider::core::MetadataStorage> metadata_storage - = storage_factory->provide_metadata_storage(); - std::unique_ptr<spider::core::DataStorage> data_storage - = storage_factory->provide_data_storage(); + auto [storage_factory, metadata_storage, data_storage] + = spider::test::create_standard_factory_resources<TestType>();
160-161: Switching tostd::shared_ptr.
Here, the test uses shared pointers whereas the previous tests use unique pointers. Consider consistent pointer types across all tests to avoid confusion or ownership mismatches.src/spider/worker/worker.cpp (1)
116-116: Consider using an initialization list.
A static analysis hint suggests that a variable is assigned in the constructor body rather than by using a member initializer list. If this is relevant in one of your constructors, consider refactoring to the initialization list for potential performance and clarity improvements.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/scheduler/scheduler.cpp (2)
73-74: Refactored heartbeat connections
Fetching the connection via the factory and using a variant for error handling clarifies the logic. Passing the dereferenced connection toupdate_heartbeatis concise. Ensure that all callers handle potential failures consistently.Also applies to: 83-84, 87-87, 93-94, 97-97
111-112: Clean-up loop storage usage
The same pattern of obtaining a connection fromstorage_factoryand verifying success or error fosters readability and uniformity. Consider adding detailed logs for maintenance tasks (e.g. referencing the scheduler ID in success logs) to improve operational observability.Also applies to: 121-122, 125-125, 130-131, 134-134, 139-139, 141-141
src/spider/client/Job.hpp (2)
96-97: Dynamic connection retrieval for job status
Retrieving connections on demand avoids storing a persistent connection unnecessarily. Thestd::moveusage with the variant is correct.Also applies to: 101-101, 103-103
134-135: get_result() refactor
Similar logic for on-demand connection retrieval. Consider unifying the connection acquisition pattern (e.g., a small helper method) to reduce repeated variant checks.Also applies to: 139-140, 141-141
src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (1)
47-48: Private constructor receiving a StorageConnection&
Marking it explicit ensures no accidental conversions. Ensuring that only your factory can instantiate this class eliminates confusion.tests/storage/test-MetadataStorage.cpp (1)
29-31: Consider extracting shared setup into a fixture.
These lines repeatedly create a StorageFactory and could be reorganized into a reusable fixture to enhance code clarity.tests/storage/test-DataStorage.cpp (1)
23-27: Consider extracting shared test setup into a fixture.
These lines repeatedly create a storage factory and could be centralized for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
src/spider/CMakeLists.txt(2 hunks)src/spider/client/Data.hpp(5 hunks)src/spider/client/Driver.cpp(3 hunks)src/spider/client/Driver.hpp(6 hunks)src/spider/client/Job.hpp(7 hunks)src/spider/client/TaskContext.cpp(3 hunks)src/spider/client/TaskContext.hpp(6 hunks)src/spider/core/DataImpl.hpp(1 hunks)src/spider/core/TaskContextImpl.hpp(2 hunks)src/spider/scheduler/FifoPolicy.cpp(4 hunks)src/spider/scheduler/FifoPolicy.hpp(2 hunks)src/spider/scheduler/SchedulerServer.cpp(2 hunks)src/spider/scheduler/SchedulerServer.hpp(2 hunks)src/spider/scheduler/scheduler.cpp(9 hunks)src/spider/storage/DataStorage.hpp(0 hunks)src/spider/storage/JobSubmissionBatch.hpp(1 hunks)src/spider/storage/MetadataStorage.hpp(0 hunks)src/spider/storage/StorageConnection.hpp(1 hunks)src/spider/storage/StorageFactory.hpp(1 hunks)src/spider/storage/mysql/MySqlConnection.cpp(2 hunks)src/spider/storage/mysql/MySqlConnection.hpp(2 hunks)src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp(1 hunks)src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp(3 hunks)src/spider/storage/mysql/MySqlStorage.hpp(4 hunks)src/spider/storage/mysql/MySqlStorageFactory.cpp(1 hunks)src/spider/storage/mysql/MySqlStorageFactory.hpp(1 hunks)src/spider/worker/FunctionManager.hpp(3 hunks)src/spider/worker/WorkerClient.cpp(2 hunks)src/spider/worker/WorkerClient.hpp(3 hunks)src/spider/worker/task_executor.cpp(3 hunks)src/spider/worker/worker.cpp(15 hunks)tests/client/test-Driver.cpp(3 hunks)tests/scheduler/test-SchedulerPolicy.cpp(8 hunks)tests/scheduler/test-SchedulerServer.cpp(4 hunks)tests/storage/StorageTestHelper.hpp(1 hunks)tests/storage/test-DataStorage.cpp(3 hunks)tests/storage/test-MetadataStorage.cpp(14 hunks)tests/worker/test-FunctionManager.cpp(5 hunks)tests/worker/test-TaskExecutor.cpp(10 hunks)tools/scripts/storage/init_db.sql(1 hunks)
💤 Files with no reviewable changes (2)
- src/spider/storage/MetadataStorage.hpp
- src/spider/storage/DataStorage.hpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
tests/client/test-Driver.cpp
[error] 37-37: syntax error
(syntaxError)
src/spider/scheduler/scheduler.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
tests/worker/test-TaskExecutor.cpp
[error] 61-61: syntax error
(syntaxError)
src/spider/client/Driver.cpp
[performance] 28-28: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/worker/worker.cpp
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
tests/storage/test-DataStorage.cpp
[error] 65-65: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (284)
src/spider/storage/JobSubmissionBatch.hpp (1)
15-16: Nice improvement: the JobSubmissionBatch is now movableMaking JobSubmissionBatch movable by defaulting the move constructor and move assignment operator enhances the flexibility of your storage API. This change is crucial for the abstract factory pattern implementation as it allows factory methods to return unique ownership of JobSubmissionBatch instances.
This modification aligns perfectly with modern C++ idioms for resource management, facilitating the transfer of object ownership without unnecessary copying.
src/spider/core/DataImpl.hpp (2)
8-9: Changed includes to support the abstract factory patternThe inclusion of
"../storage/StorageFactory.hpp"and the switch to"Data.hpp"properly set up the dependencies required for the storage factory implementation.
16-22: StorageFactory parameter correctly integrated into create_data methodThis change properly integrates the StorageFactory into the data creation process, allowing client code to remain agnostic about the specific storage backend implementation. The method now forwards the factory to the Data constructor, which is essential for the abstract factory pattern.
This modification enhances modularity by making the concrete storage implementation a runtime decision rather than a compile-time dependency.
src/spider/scheduler/SchedulerServer.hpp (3)
31-32: Improved ownership semantics with shared_ptr for StorageConnectionChanging the constructor parameter from a reference to a shared_ptr is consistent with the abstract factory pattern, which typically returns smart pointers for ownership transfer.
49-49: Member variable properly updated to match constructor parameterThe m_conn member variable has been correctly updated to match the constructor parameter type, ensuring consistent ownership semantics throughout the class.
30-50:Details
✅ Verification successful
Verify access to StorageConnection methods
Since the connection is now a shared_ptr, ensure that all accesses to m_conn in the implementation file properly dereference the pointer when calling methods.
🏁 Script executed:
#!/bin/bash # Check for potential pointer dereferencing issues in SchedulerServer.cpp rg -A 3 -B 3 "m_conn\." src/spider/scheduler/SchedulerServer.cpp || echo "No direct member access found without dereferencing" rg -A 3 -B 3 "m_conn->" src/spider/scheduler/SchedulerServer.cpp || echo "No arrow operator access found"Length of output: 317
Review Approved: No Pointer Dereferencing Issues Detected
The grep checks confirm that there are no instances in
src/spider/scheduler/SchedulerServer.cppwherem_connis accessed without proper dereferencing. All accesses appear to be correctly handled, and no misuse of the shared pointer was detected.src/spider/CMakeLists.txt (4)
4-5: New source files correctly added for MySQL storage factory implementationThe MySQL-specific factory implementation source files are properly included in the build configuration, supporting the abstract factory pattern.
32-33: MySQL storage factory header added to core headersThe MySqlStorageFactory.hpp header is correctly added to the core headers list, which is important for compilation.
36-36: Abstract StorageFactory header properly addedThe addition of StorageFactory.hpp to the core headers is essential for the abstract factory pattern implementation. This header likely contains the interface that concrete factory implementations must follow.
1-178:Details
✅ Verification successful
Verify storage factory integration in client code
To ensure the complete integration of the abstract factory pattern, it would be beneficial to verify how the storage factory is created and used in the client code.
🏁 Script executed:
#!/bin/bash # Look for instances where StorageFactory is created or used echo "Checking for StorageFactory creation patterns:" rg -A 5 "std::make_shared<.*StorageFactory" --type cpp echo -e "\nChecking for StorageFactory usage in client code:" rg -A 3 "storage_factory" --type cpp src/spider/client/Length of output: 16053
StorageFactory Integration Verified in Client Code
The verification shows that the abstract factory pattern is correctly implemented. The creation of the
MySqlStorageFactoryviastd::make_sharedand its subsequent usage for providing metadata, data, and storage connections are consistently integrated in both the worker and client code (e.g. insrc/spider/worker/worker.cpp,src/spider/worker/task_executor.cpp, and across various files insrc/spider/client/). No further changes are required.src/spider/worker/task_executor.cpp (3)
27-28: Includes look fine
Including"MySqlStorageFactory.hpp"and"StorageFactory.hpp"aligns with the new factory‑based architecture.
126-127: Verify the base class destructor
When storing polymorphic objects in astd::shared_ptr<StorageFactory>, ensure that the base class (StorageFactory) has a virtual destructor to prevent potential memory leaks.
173-174: New parameter pass is consistent
Passingmetadata_storeandstorage_factorytocreate_task_context()aligns with the updated design. Implementation seems coherent.src/spider/storage/mysql/MySqlConnection.cpp (1)
19-20: Improved memory ownership
Switching to returningstd::unique_ptr<StorageConnection>is a solid strategy for managing lifetime and avoiding copies.src/spider/scheduler/FifoPolicy.cpp (1)
66-66: Shared pointer param promotes flexibility
Accepting astd::shared_ptr<core::StorageConnection>in the constructor aligns well with the new design for backend abstraction.src/spider/storage/StorageConnection.hpp (1)
6-14: Solid approach to managing copy and move semantics
The explicit deletion of the copy constructor and copy assignment operator, alongside default move semantics, ensures unique ownership while still permitting resource transfers. Declaring a virtual destructor is also a good design choice for proper cleanup in a polymorphic context.src/spider/scheduler/SchedulerServer.cpp (2)
33-40: Shared pointer approach aligns with abstract factory pattern
Switching tostd::shared_ptr<core::StorageConnection>ensures consistent and flexible resource sharing across the scheduler’s components. The move intom_connis correct for ownership transfer, provided all references to this connection are carefully managed to avoid concurrency hazards.
142-150: Dereferencing the shared connection is correct
Accessing the connection through*m_connintegrates well with the new factory-based architecture. The logic to retrieve and reset the job with the metadata store is properly updated to reflect the pointer-based design.src/spider/scheduler/FifoPolicy.hpp (1)
25-37: Consistent transition to shared pointers
Updating the constructor parameter and member variable to usestd::shared_ptr<core::StorageConnection>matches the broader refactor strategy. This design is more extensible, particularly if multiple schedulers or policies share the same storage object.src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp (2)
38-57: Transactional batch submission logic looks robust
Executing the batched statements in sequence, rolling back on errors, and committing upon success is well-structured. Returning aStorageErron failure is clear and facilitates consistent error handling downstream.
13-36:Details
✅ Verification successful
Confirm casting safety for cross-backend usage
Usingstatic_cast<MySqlConnection&>(conn)depends on the guarantee that thisStorageConnectionis always aMySqlConnection. If the factory can produce a different backend, this cast could invoke undefined behaviour. You may wish to verify or assert the derived type before casting.
🏁 Script executed:
#!/bin/bash # Validate that no usage attempts to pass a storage connection other than MySqlConnection # to MySqlJobSubmissionBatch. We will search for relevant creation sites. rg --ignore-case "MySqlJobSubmissionBatch"Length of output: 2804
Type Casting is Currently Safe but Consider a Defensive Check
After verifying that instances of
MySqlJobSubmissionBatchare only created by MySQL-specific factory code (as seen inMySqlStorageFactory.cpp), it appears that the guaranteed invariant is maintained—only aMySqlConnectionis ever passed to the constructor. However, while the use ofstatic_castis safe given this context, it is recommended that you add a defensive runtime check (for example, using anassertwith adynamic_cast) to ensure that this invariant holds even if the code is extended in the future.
- Location:
src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp(constructor)- Recommendation: Consider adding a runtime assertion such as:
to explicitly document and enforce the requirement.assert(dynamic_cast<MySqlConnection*>(&conn) && "Expected a MySqlConnection");src/spider/worker/WorkerClient.hpp (1)
14-14: No issues with including the StorageFactory header.
This addition is consistent with the shift towards the abstract factory pattern.src/spider/storage/mysql/MySqlConnection.hpp (3)
16-18: Forward declaration of MySqlStorageFactory is good.
This approach reduces coupling and allows the friend declaration without a full include.
35-37: Factory creation method returning a unique pointer is an improvement.
Returning astd::unique_ptr<StorageConnection>fosters safer memory management in the factory design.
41-42: Friend class declaration is clear.
GrantingMySqlStorageFactoryprivate access appears necessary for connection instantiation details.src/spider/worker/FunctionManager.hpp (5)
29-29: Swapping out the MySQL-specific header for a generic StorageConnection header is appropriate.
This reduces dependency on a concrete backend and supports the new abstraction.
286-287: Variant-based error handling is sound.
Creating the storage connection viaprovide_storage_connection()aligns with the new factory approach.
295-295: Unique pointer extraction is implemented correctly.
Moving from the variant ensures exclusive ownership of the StorageConnection instance.
305-305: Dereferencing the storage connection is correct.
You have already handled the possibility of errors beforehand, so this usage should be safe.
310-315: Dynamic data creation matches the new abstract design.
CombiningDataImpl::create_datawith the storage factory fosters flexible data instantiation.src/spider/client/TaskContext.cpp (4)
12-13: Includes for storage abstractions look appropriate.
Thank you for correctly adding the necessary headers forStorageConnectionandStorageFactory. This helps decouple the task context from specific database implementations.
23-31: Good use of variant-based connection retrieval in kv_store_get.
Retrieving a connection viastd::variantand cleanly handling theStorageErrensures robust error handling before invoking the data store. Moving theunique_ptrfrom the variant is correct to maintain exclusive ownership.
42-50: Consistent error-checking logic in kv_store_insert.
The repeated approach of retrieving the connection and confirming noStorageErris raised provides consistency. This pattern makes the insertion step straightforward and reliable.
57-66: Appropriate retrieval pattern in get_jobs.
Once again, the code neatly obtains a connection through the factory. The design is consistent and uses ownership transfer correctly. Good job maintaining uniform error handling and usage of the metadata store.src/spider/core/TaskContextImpl.hpp (3)
11-11: Header reference for StorageFactory is well-introduced.
IncludingStorageFactory.hppcentralises factory definitions and ensures clarity in subsequent usage.
19-22: Refined create_task_context signature.
Passing the newstorage_factoryreference improves extensibility. This design supports multiple backends without changes to the client code, aligning well with the abstract factory principle.
33-37: Convenient accessor for storage factory.
The staticget_storage_factorymethod is a clean addition that exposes the underlying factory in a controlled manner. This adheres to object-oriented design and fosters easy retrieval in other parts of the system.src/spider/client/Driver.hpp (6)
22-22: Switched to a more abstract include for storage.
Replacing direct MySQL includes withStorageFactory.hpphelps reduce coupling and fosters structural flexibility.
85-91: Augmented get_data_builder with factory and connection.
Providingm_storage_factoryandm_connto theDataBuildergrants it the adaptability to create storage-specific objects as needed, following the factory approach.
155-155: Batch submission now factory-based.
Acquiring aJobSubmissionBatchthroughm_storage_factoryeliminates direct MySQL dependencies here, streamlining future expansions to additional backends.
229-235: Refactored start method transitions to factory pattern (Task version).
Delivering them_storage_factoryandm_connto theJobconstructor is consistent with the rest of the code. This ensures that all job logic remains backend-neutral.
279-285: Refactored start method transitions to factory pattern (TaskGraph version).
Similar to the single-task version, passing the factory clarifies the job lifecycle and underlying storage handling. Good job preserving the existing flow while enabling a new backend approach.
310-310: Introduction of the factory pointer in Driver.
Addingm_storage_factorycaptures the new abstraction neatly in the driver. This central point further simplifies changing or extending storage backends.tests/storage/StorageTestHelper.hpp (4)
7-12: Well-structured header importsThe updated imports properly reflect the shift towards the abstract factory pattern, removing direct dependencies on concrete implementations and adding the necessary factory-related headers. This improves modularity and makes it clearer what dependencies this file actually requires.
15-16: Good type update for storage URLThe change from
char const* consttostd::string constfor storage URL is appropriate. Usingstd::stringprovides better type safety and more flexibility when working with string operations that might be needed with URLs.
18-18: Clear type definition for StorageFactoryTypeListThe introduction of
StorageFactoryTypeListas a tuple containingcore::MySqlStorageFactorysets up a clean foundation for future storage backend extensions. Additional factory types can be easily added to this tuple when new storage backends are implemented.
20-24: Correctly constrained factory creation functionThe
create_storage_factorytemplate function is properly constrained to work only with theMySqlStorageFactorytype. This function returns a base class pointer (StorageFactory), adhering to the abstract factory pattern principles by hiding concrete implementation details.src/spider/storage/StorageFactory.hpp (2)
4-12: Good header organizationThe includes are well-organized, with system headers first followed by project headers. All necessary dependencies for the factory interface are properly included.
13-23: Well-designed abstract factory interfaceThe
StorageFactoryinterface is properly designed with pure virtual methods and a virtual destructor. The use of modern C++ features likestd::unique_ptrandstd::variantis excellent for type safety and resource management.The
provide_storage_connectionmethod returns a variant that can either be a connection or an error, which is a clean approach to error handling without exceptions. This approach allows callers to explicitly handle potential connection failures.src/spider/storage/mysql/MySqlStorageFactory.hpp (3)
4-14: Well-structured header importsThe header imports are properly organized with system headers first, followed by project headers. All necessary dependencies for implementing the MySQL storage factory are included.
15-26: Good implementation of the StorageFactory interfaceThe
MySqlStorageFactoryclass correctly inherits fromStorageFactoryand overrides all the pure virtual methods. The explicit constructor taking a URL string is appropriate for initialization purposes.
27-29: Proper encapsulation of URLStoring the URL as a private member variable maintains proper encapsulation. Consider whether it should be stored by value or by reference depending on the typical lifetime of the URL string.
tests/scheduler/test-SchedulerServer.cpp (5)
28-29: Good addition of storage abstractionsThe inclusion of
StorageConnection.hppandStorageFactory.hppproperly reflects the move to the abstract factory pattern. This reduces direct dependencies on specific storage implementations in the test code.
40-46: Appropriate use of factory patternThe test case now correctly uses the abstract factory pattern. It creates a storage factory first and then uses it to obtain the necessary storage components. This is a good implementation of the pattern and makes the test more flexible for future storage backends.
48-53: Well-handled error checking for connectionThe code properly handles the variant return type from
provide_storage_connection()usingstd::holds_alternativeto check for success before extracting the connection. This is a robust approach to error handling.
85-85: Proper connection usageThe updated code correctly dereferences the connection pointer when calling
add_jobon the metadata store. This change maintains consistency with the new factory-based approach.
100-100: Proper connection usage for remove_jobSimilar to the
add_jobcall, this correctly dereferences the connection pointer when callingremove_job, maintaining consistency with the new approach.tests/scheduler/test-SchedulerPolicy.cpp (10)
24-25: Ensure consistent usage of includes.
IncludingStorageConnection.hppandStorageFactory.hppis correct and aligns well with the new abstract factory approach. No issues found.
32-32: Good approach to test multiple storage implementations.
Utilizingspider::test::StorageFactoryTypeListfor parameterized tests is a solid strategy that promotes reusability and ensures coverage of various storage backends.
83-84: Consistent cleanup ensures data integrity.
Removing the job after testing is a good practice, preventing leftover data from polluting subsequent tests. No further issues found.
94-94: Well-structured parameterization.
Leveragingspider::test::StorageFactoryTypeListagain confirms consistency across tests. Overall design is coherent.
102-106: Variant usage for storage connection.
Using a variant to handle errors and valid connections is clean. The subsequent hold check forstd::unique_ptrprevents exceptions.
139-139: Keeps the code flexible.
Parameterizing scheduler tests with multiple storage backends viaStorageFactoryTypeListencourages maintainability. No issues noted.
151-156: Proper error-checking for connection initialization.
The pattern for retrieving connections is consistent with previous tests. The repeated checks for successful connections are appropriate.
172-172: Job creation success.
No issues found. This helps confirm basic workflow correctness.
183-183: Clean removal of job.
Consistent tear-down for test data. Good practice.
40-44:Details
✅ Verification successful
Confirm ownership hand-off from unique_ptr to shared_ptr.
Converting the unique_ptr connection into a shared_ptr is acceptable, but ensure consistent ownership semantics. If future concurrency or extended lifetime is required, re-check usage to avoid subtle ownership bugs.
🏁 Script executed:
#!/bin/bash # Validate whether multiple references to the same StorageConnection # exist in the codebase, which might cause ownership complexity. rg --context 3 'std::shared_ptr<spider::core::StorageConnection>'Length of output: 3649
Ownership hand-off verified and consistent.
The code consistently converts a unique_ptr to a shared_ptr across both test files and the scheduler source. The grep search confirmed that this pattern is applied uniformly, suggesting that the ownership hand-off is correctly implemented. There appears to be no immediate risk of ownership or lifetime issues; however, please remain vigilant if the use cases evolve to require concurrent access or extended lifetimes.tests/client/test-Driver.cpp (5)
17-21: Template-based test expansions.
Replacing multipleTEST_CASEs withTEMPLATE_LIST_TEST_CASEis a good approach to extend coverage over different backends. No problems found.
22-23: Storage URL retrieval looks correct.
Usingget_storage_url<TestType>()to retrieve the storage URL retains flexibility. TheDrivercreation is straightforward.
37-39: Confirm the syntax error warning.
The static analysis warning about a syntax error seems to be a false positive. The code compiles as expected.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 37-37: syntax error
(syntaxError)
54-60: Expanded testing for bind task.
Adapting these tests to a templated approach provides robust coverage. No immediate issues found.
67-73: Compositional testing with data structures.
The addition of data-based tasks broadens test scenarios. These changes are consistent and likely improve coverage.src/spider/worker/WorkerClient.cpp (4)
27-28: Correct includes for the factory-based design.
AddingStorageConnection.hppandStorageFactory.hppaligns perfectly with the new abstract factory approach.
36-37: Constructor now supports a storage factory.
Enriching the constructor withmetadata_storeandstorage_factoryis in line with the abstract factory pattern for easier testability.
51-52: Separate error state from valid connection.
Using a variant to differentiateStorageErrfromStorageConnectionis a clean approach that simplifies error handling.
130-134: Create task instance flow.
Callingcreate_task_instancewith*connclarifies usage of the underlying object. No functional issues noted.src/spider/storage/mysql/MySqlStorageFactory.cpp (1)
27-35: Good use ofstd::variantfor handling connection creation.The logic for returning either
StorageConnectionorStorageErris clear and provides robust error-checking at runtime.tests/worker/test-TaskExecutor.cpp (7)
61-65: Use of parameterised tests is commendable.Switching to
TEMPLATE_LIST_TEST_CASEimproves coverage by checking multiple storage backends easily.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 61-61: syntax error
(syntaxError)
93-97: Good approach withTEMPLATE_LIST_TEST_CASE.This ensures consistency for verifying different storage factory implementations.
123-127: UsingTEMPLATE_LIST_TEST_CASEfor error handling tests is beneficial.Examining failures across multiple storage backends helps verify uniform error handling logic.
156-157: Adoption of a parameterised test for data arguments is excellent.Enabling the test to handle various backends fosters a more robust suite.
165-168: Clear and concise usage ofstd::variant.This pattern neatly handles successful and erroneous connection creation, ensuring the test fails fast upon error.
177-178: Consistent factory usage for adding driver and driver data.Ensuring the test calls
add_driverandadd_driver_datawith the same factory-based connection logic helps validate the new architecture.
206-206: Clean-up after the test is prudent.Removing data with
remove_data(*conn, data.get_id())preserves test isolation and prevents side effects.src/spider/client/Driver.cpp (2)
19-20: Appropriate includes for MySQL-based storage factory components.The includes align with the new abstract factory approach without polluting other modules.
25-26: Abstract factory usage fosters easier backend substitution.By invoking
MySqlStorageFactorywithinDriver, the code centralises storage logic, helping streamline any future backend replacements.Also applies to: 30-31, 33-34, 38-38
tests/worker/test-FunctionManager.cpp (10)
20-23: Include the new storage headers.
These new includes align with the transition to the abstract factory pattern. No issues observed.
69-69: Use ofStorageFactoryTypeList.
Switching from a backend-specific list toStorageFactoryTypeListappropriately supports multiple storage backends in tests.
71-77: Instantiating storage via factory.
Usingcreate_storage_factory<TestType>()and retrieving storage interfaces throughprovide_data_storage()andprovide_metadata_storage()improves modularity. Looks consistent with the factory approach.
82-83: Passing the factory toTaskContext.
Injectingstorage_factoryintoTaskContextcentralizes storage concerns and streamlines resource creation.
128-129: Expanding tests to factory-based storage.
Updating the type list for the template test ensures broader coverage of the new storage abstraction.
141-142: Passing storage components toTaskContext.
This approach is clean. No concerns at present.
164-168: Providing data and metadata storage.
The usage ofprovide_metadata_storage()andprovide_data_storage()continues the factory pattern. Looks good.
169-172: Connection variant usage.
Extracting the connection fromstd::variantis a neat way to handle errors. This code correctly checks for storage errors.
180-181: Verifying driver addition.
Using.success()to confirm operation success ensures test correctness. Good job.
198-198: Data clean-up.
Removing test data ensures a tidy environment, preventing clutter for subsequent tests.src/spider/client/TaskContext.hpp (5)
22-23: New storage headers.
IncludingStorageConnection.hppandStorageFactory.hppaligns with the design shift to a storage-agnostic approach.
63-68: Enhancingget_data_builder.
Forwarding thestorage_factoryinto the data builder fosters deeper integration with the new factory approach. Implementation looks clean.
164-168: Connection variant creation.
Constructing the storage connection viaprovide_storage_connection()abstracts away backend details. Excellent design.
244-245: Constructor signature change.
Adding the factory to the constructor consolidates all storage dependencies in one place, improving maintainability.
260-260: Tracking the factory reference.
m_storage_factoryas astd::shared_ptris consistent with other storage references.src/spider/worker/worker.cpp (15)
41-43: Replacing MySQL-only headers with factory-based includes.
ImportingMySqlStorageFactory.hppalongsideStorageFactory.hppandStorageConnection.hppsupports the new abstract storage pattern.
103-139:heartbeat_looprefactoring with factory usage.
Passing the storage factory as a parameter and extracting the connection from astd::variantis a robust design. The improved error messages (“Failed to connect to storage: ...”) are clearer.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
225-228: Refactoredtask_loopparameters.
Receivingstorage_factoryandstorage_urlin the function signature decouples the worker from a single backend, consistent with your abstract factory model.
251-261: Acquiring the storage connection intask_loop.
Usingstd::variantto detect and handle connection errors elegantly avoids throwing exceptions. Nicely done.
263-279: Failing tasks on invalid arguments.
Callingmetadata_store->task_fail(*conn, ...)ensures the system logs and tracks the failure. Correct approach.
288-289: Usingstorage_urlin theTaskExecutorconstructor.
Passing the URL for dynamic decisions is consistent with your new factory-based architecture.
296-307: Reusing the storage connection.
The updated code rechecks connectivity after task execution. Good strategy for reliability and error handling.
311-314: Task failure logging.
The revised error message ensures consistent feedback when tasks fail.
325-325: Task failure on parsing errors.
Ensuring all failure conditions are properly recorded helps with debugging. No concerns here.
340-340: Failing task on output parsing issues.
Capturing the parsing error intask_failis consistent with the approach used for arguments.
354-354: Finishing the task in metadata.
task_finish(*conn, instance, outputs)finalizes the job properly. Good alignment with the abstract storage design.
417-422: Creating factory-based storage inmain.
BuildingMySqlStorageFactoryand callingprovide_metadata_storage()/provide_data_storage()ensures the worker remains backend-agnostic.
429-442: Registering the driver.
Adding the driver with a brand-new connection is straightforward. If the driver requires further initialization, you can easily extend this approach.
451-452: InstantiatingWorkerClient.
ConstructingWorkerClientwith references to all factory-based resources fosters consistency across the worker.
462-472: Passing factory references to threads.
Providing the factory to bothheartbeat_loopandtask_loopfosters the new design’s flexibility. This is cleanly done.src/spider/scheduler/scheduler.cpp (4)
27-29: Consolidated includes for storage abstraction
These includes cleanly transition from MySQL-specific headers toward a higher-level storage abstraction. This step is aligned with the new factory design. No issues found.
195-196: Instantiating storage factory and storages
Switching toMySqlStorageFactorybehind theStorageFactoryinterface properly decouples the scheduler’s main function from backend details. The calls toprovide_metadata_storage()andprovide_data_storage()are straightforward.Also applies to: 197-198
202-203: Establishing the initial storage connection
Using a shared pointer forconnafter moving from the variant is correct and ensures the connection persists for the subsequent operations (initializeandadd_scheduler). The error handling flow looks good.Also applies to: 210-211, 213-213, 218-218, 237-237
247-247:Details
❓ Verification inconclusive
Thread creation for heartbeat and cleanup
Passingstorage_factoryby reference into each thread is reasonable. Verify that the lifetime ofstorage_factoryoutlasts these threads to avoid potential invalid references.Also applies to: 256-260
🏁 Script executed:
#!/bin/bash # Confirm references to `storage_factory` do not outlive it: rg -A 10 "std::cref(storage_factory)"Length of output: 39
Attention: Please confirm the lifetime of
storage_factoryThe current implementation passes
storage_factoryby reference usingstd::creffor thread creation (heartbeat and cleanup). It remains crucial to ensure that its lifetime clearly exceeds the lifetime of these threads to prevent any dangling reference issues. In addition to verifying the usage at line 247, please double-check the similar pattern at lines 256–260.
- Verify that
storage_factoryis declared in a scope that outlasts the creation and execution of these threads.- Confirm that there’s no risk of the object being destroyed or going out of scope while the threads are still using the reference.
- Re-assess any related synchronisation logic that might affect the object's lifetime.
Once you’ve confirmed that these conditions are met, please update the code documentation accordingly for clarity.
src/spider/client/Job.hpp (4)
25-25: New include for storage factory
IncludingStorageFactory.hppdirectly reflects the shift to a more generic factory model.
68-69: Factory-based connection in wait_complete()
The pattern of obtaining aStorageConnectionfrom the factory ifm_connis null is clear. Good job ensuring the code does not proceed on error.Also applies to: 73-74
163-164: Constructors adapted with storage factory
Storingm_storage_factoryclarifies how the job obtains fresh connections. The initialization list usage is consistent.Also applies to: 166-167, 172-172, 177-177
310-311: Passing storage factory into DataImpl
Extending thecreate_datacall withm_storage_factoryfosters more dynamic or remote data lookups if needed. This is a clean extension of your pattern.src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (4)
13-16: Forward declaration of MySqlStorageFactory
This forward declaration helps avoid circular dependencies. Good approach.
19-22: Deleted copy constructor and defaulted move constructor
This enforces that each batch is unique. This pattern is ideal when managing raw resources (e.g., prepared statements).
24-24: submit_batch override
Explicitly overridingsubmit_batchwith aStorageConnection&argument aligns perfectly with the new abstract approach. Implementation details will presumably handle the MySQL specifics.
59-59: Friend class MySqlStorageFactory
Granting factory access is consistent with the intended design pattern, enabling precise lifecycle control overMySqlJobSubmissionBatch.tests/storage/test-MetadataStorage.cpp (80)
33-33: LGTM
No issues found; the metadata storage creation call is appropriate.
35-38: LGTM
The variant usage and retrieval of the StorageConnection pointer are correct.
45-45: LGTM
Adding a driver through the new connection reference looks correct.
49-49: LGTM
Heartbeat timeout check is appropriately verified.
58-58: LGTM
Timeout behaviour logic is clear and consistent with the rest of the test.
66-66: LGTM
The update heartbeat method call is used correctly.
68-68: LGTM
The repeated heartbeat timeout check ensures state transitions are validated properly.
74-74: LGTM
New test suite name accurately reflects the scope of storage tests.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 74-74: syntax error
(syntaxError)
79-80: LGTM
Factory creation code is consistent with the preceding template list tests.
82-82: LGTM
Metadata storage instantiation is correctly obtained from the factory.
84-85: LGTM
Variant usage pattern follows best practices.
86-87: LGTM
Connection retrieval and pointer extraction logic are correct.
94-94: LGTM
Adding the scheduler with the newly created connection reference is handled properly.
100-100: LGTM
Ensuring that the scheduler address is retrieved successfully is a good test.
106-106: LGTM
KeyNotFoundErr confirms proper error handling for missing scheduler records.
110-110: LGTM
Retrieving the default scheduler state is tested properly.
116-116: LGTM
The scheduler state update path appears correct.
119-119: LGTM
Verifying the new scheduler state ensures correct state transition.
123-123: LGTM
New test suite title clarifies it’s about job batch operations.
125-126: LGTM
The repeated factory creation code is consistent with the prior pattern in this file.
128-128: LGTM
Metadata storage creation still appears correct in this context.
131-131: LGTM
DataStorage or MetadataStorage are not directly used here, but the code is consistent for job batches.
133-133: LGTM
Variant usage for establishing the connection is the same tested pattern as above.
134-136: LGTM
Ensuring the connection is available before continuing is a reliable approach.
138-139: LGTM
Creation of a JobSubmissionBatch object from the factory matches the new design.
191-194: LGTM
Adding jobs in batch, then submitting the batch, properly tests the unified batch approach.
198-198: LGTM
Search for jobs by client ID is tested thoroughly.
202-203: LGTM
Ensuring an empty result for unknown clients is correct.
210-210: LGTM
Job metadata retrieval is properly verified.
211-211: LGTM
Assertions confirm correct job ID and client ID.
220-221: LGTM
Loading the task graph from storage and verifying graph equality is a solid test.
223-224: LGTM
Simple job’s graph retrieval is also tested thoroughly.
228-228: LGTM
Confirming retrieval of the child task verifies the stored data is consistent.
233-233: LGTM
Fetching child tasks from a given parent ensures parent-child relationships are correct.
239-240: LGTM
Ensuring that get_parent_tasks returns the correct parents is vital to verifying graph structure.
248-248: LGTM
Remove job call is tested thoroughly.
249-249: LGTM
Validating that the job removal returns success is appropriate.
251-251: LGTM
Confirming KeyNotFoundErr on retrieving a removed job’s graph ensures correctness.
253-254: LGTM
Re-checking the other job is still present is good for partial removal testing.
255-255: LGTM
Removing the second job finalizes the test scenario of batch removal.
263-264: LGTM
New “Job add, get and remove” test suite is consistent with the overall approach.
266-266: LGTM
Creating the metadata storage again within this test ensures isolation between tests.
268-271: LGTM
Verifying the connection setup logic remains consistent with prior tests.
279-289: LGTM
Building a complex task graph is a robust approach to ensure broad coverage.
320-321: LGTM
Simple task graph creation and verification are consistent with earlier patterns.
323-325: LGTM
The calls to add_job are consistent with the new approach.
328-328: LGTM
Retrieving jobs by an unknown client is handled properly.
332-333: LGTM
Checking that the correct jobs were returned for a known client is well tested.
341-343: LGTM
Job metadata verification ensures the correct data is stored.
363-364: LGTM
Confirming we can retrieve a parent’s child tasks is consistently tested.
369-369: LGTM
Ensuring get_parent_tasks returns the correct set of parent tasks for the child.
379-380: LGTM
Removing the simple job tests partial cleanup.
381-381: LGTM
Confirming KeyNotFoundErr after removal is correct.
383-384: LGTM
Ensuring the other job’s graph remains after partial removal is validated.
385-385: LGTM
Final removal of the second job completes the scenario.
388-390: LGTM
New “Task finish” test suite name is suitable for verifying job progression logic.
392-392: LGTM
Again, re-initializing metadata storage ensures test independence.
394-397: LGTM
Connection retrieval pattern remains consistent.
429-430: LGTM
Running state set prior to finishing the task is a correct approach to test transitions.
433-434: LGTM
Finishing one parent ensures no effect on sibling tasks or the child.
447-447: LGTM
Similarly marking the second parent’s state to Running ensures consistent testing of transitions.
449-450: LGTM
Finishing the second parent triggers the child state to become ready.
452-453: LGTM
Verifying the child’s input after both parents finish is crucial for correctness.
461-463: LGTM
New “Job reset” test suite name clarifies that these tests target resetting states.
465-465: LGTM
Re-initializing metadata storage for reset scenario is consistent.
467-470: LGTM
Acquiring the connection to test reset functionality is part of the uniform approach.
502-503: LGTM
Adding the job to test reset logic is correct.
506-507: LGTM
Marking the parent as Running before finishing it is consistent with prior patterns.
509-509: LGTM
Finishing the parent task confirms normal progression once again.
510-511: LGTM
Handling output data for the parent’s finish is tested thoroughly.
521-521: LGTM
Transitioning the child to Running is the correct next step in the sequence.
523-524: LGTM
Child finishing ensures the entire job is set to a complete state prior to reset testing.
527-528: LGTM
Verifying the reset_job call ensures the tasks return to their appropriate states.
532-533: LGTM
Parent tasks returning to ‘Ready’ with inputs intact is correctly validated.
539-539: LGTM
Additional checks for missing output data after the reset are correct.
540-540: LGTM
The second parent remains ‘Ready’ with no output set, consistent with the reset logic.
545-545: LGTM
Confirming no output is present for the second parent after reset is correct.
546-546: LGTM
Checking the child’s state is ‘Pending’ post-reset ensures proper job-level reset.
547-547: LGTM
Ensuring child’s inputs are cleared after reset is a key part of state re-initializaton.
555-555: LGTM
Removing the job completes the test, confirming the entire reset operation works end-to-end.tools/scripts/storage/init_db.sql (18)
1-6: Consider indexing fields for performance.
The drivers table’s primary key is good. For large-scale usage, ensure queries on this table remain performant.
7-15: LGTM
Linkingschedulerstodriversvia a foreign key is appropriate for data integrity.
16-24: LGTM
Jobs table has suitable indexes for client lookups and creation time.
25-37: LGTM
Tasks table enumerations and foreign key constraints are well-defined.
38-47: LGTM
input_tasks properly references jobs and tasks, preserving cross-table relationships.
48-57: LGTM
output_tasks design matches input_tasks for symmetrical indexing.
58-65: LGTM
Data table uses varbinary for values, which is flexible for different data types.
66-76: LGTM
task_outputs table references tasks and data with no obvious structural issues.
77-90: LGTM
task_inputs referencing both tasks and output_tasks ensures accurate data flow modeling.
91-91: LGTM
Empty line or spacing is harmless here.
92-100: LGTM
task_dependencies table ensures parent-child relationships are enforced at the schema level.
101-108: LGTM
task_instances introduces a separate log of task executions with sensible defaults.
109-109: LGTM
Blank line before the next create statement is acceptable.
110-116: LGTM
data_locality manages data distribution references effectively.
117-125: LGTM
data_ref_driver clarifies references between data and driver instances.
126-134: LGTM
data_ref_task is consistent with how driver references are managed.
135-141: LGTM
client_kv_data table extends the schema for client-specific storage with a composite key.
142-149: LGTM
task_kv_data similarly tracks key-value pairs for tasks.tests/storage/test-DataStorage.cpp (44)
16-17: LGTM
Including StorageConnection and StorageFactory is essential for the new abstraction.
28-34: LGTM
Obtaining metadata and data storage from the factory aligns with the abstract factory design goal.
35-38: LGTM
The variant-based connection retrieval is consistent with the rest of the codebase.
44-45: LGTM
Adding driver data is tested thoroughly.
50-50: LGTM
Ensuring that adding data with the same ID returns DuplicateKeyErr is a crucial negative test.
54-54: LGTM
Retrieving the stored data verifies that the data insertion was successful.
58-58: LGTM
Confirming the subsequent removal of data is handled properly.
66-66: LGTM
New test scenario for driver key-value data is well-labeled.
68-68: LGTM
Reusing the templated approach for different storage factory instantiations is consistent.
70-75: LGTM
Acquiring metadata and data storage remains consistent with the tested pattern.
77-81: LGTM
Variant usage for connection retrieval is repeated and properly tested.
85-85: LGTM
Successfully adding a driver prior to storing driver KV data is correct.
89-89: LGTM
Adding client KV data ensures we can store arbitrary key-value pairs.
94-94: LGTM
Properly detects duplicate keys for the same driver.
98-100: LGTM
Retrieving the client KV data confirms the stored value’s integrity.
106-106: LGTM
New test scenario for task key-value data.
110-113: LGTM
Singleton approach for metadata and data storage remains the same.
115-118: LGTM
Acquiring the storage connection again ensures independence of tests.
128-128: LGTM
Adding a job checks that tasks exist in the system for subsequent key-value storage.
132-133: LGTM
Adding KV data for the task and verifying success covers the normal usage scenario.
137-137: LGTM
DuplicateKeyErr on repeated insert is tested thoroughly.
141-142: LGTM
Retrieving the task’s stored KV data confirms data consistency.
145-146: LGTM
Removing the job is consistent with leftover cleanup.
153-154: LGTM
New scenario for “Add and remove task reference for task.”
155-159: LGTM
Storage factory usage is consistent.
160-164: LGTM
Acquiring connection once again follows the same variant-based approach.
167-167: LGTM
Fails correctly when both the data and the task do not exist.
168-168: LGTM
Error handling for missing references is tested thoroughly.
179-179: LGTM
Adding a job ensures tasks exist for subsequent references.
185-186: LGTM
Adding task data first is correct, ensuring the data is known before referencing it.
189-189: LGTM
Successfully adding a task reference once both data and task exist.
192-192: LGTM
Removing the reference is tested to ensure clean detach.
195-196: LGTM
Removing the job ensures the test environment resets.
198-198: LGTM
remove_dangling_data call checks that unreferenced data is removed to keep storage clean.
203-203: LGTM
Confirming that retrieval of the data fails after it has been cleared.
209-209: LGTM
New scenario for “Add and remove data reference for driver.”
210-216: LGTM
Factory-based approach is repeated and consistent.
218-221: LGTM
Creating the connection with the variant-based approach again.
226-226: LGTM
Fails correctly if data or driver do not exist prior to reference creation.
231-232: LGTM
Adding an additional driver ensures multi-driver scenarios are tested.
235-235: LGTM
Again verifying an error if data is not present.
239-239: LGTM
Storing data with an existing driver is correct.
242-242: LGTM
Adding a driver reference for the second driver ensures cross-driver references are valid.
245-245: LGTM
Removing the reference to ensure the system properly disassociates data from the driver.src/spider/client/Data.hpp (7)
18-19: No issues with new includes.
These includes properly align with the new abstract factory approach.
70-81: Consider storing the newly created connection.
Whenm_connectionis null, the logic uses a newly created connection only within this method call and does not store it inm_connection. This can lead to repeated new connections ifset_localityis called multiple times. Confirm that this repeated connection creation is intentional for your design goals.
124-132: Local conn variable is not updated in the class state.
You create a temporaryconnifm_connectionis null, but never updatem_connection. This may lead to future calls not reusing the newly created connection. Ensure that either ephemeral usage is intended or store this connection inm_connection.
157-176: Constructor aligns well with the abstract factory pattern.
The introduction of these builder constructors, explicitly accepting storage references, is consistent with the PR’s aim to refactor storage creation into a factory. No immediate concerns.
195-201: Private constructor without a shared StorageConnection pointer.
This constructor is reasonable for scenarios where you do not want to embed a connection in the constructed object. No pressing issues.
202-209: Private constructor with a shared StorageConnection pointer.
Allowing an existing connection to be provided supports reusability. Looks consistent with the updated design of the Data class.
215-216: Added member variables for factory and connection.
Storing them as shared pointers is consistent with the new abstraction pattern and avoids potential memory ownership complications.src/spider/storage/mysql/MySqlStorage.hpp (7)
31-32: Forward declaration for the future.
DeclaringMySqlStorageFactoryis a neat approach to avoid cyclic dependencies. No issues.
34-39: Default copy and move ctors explicitly enabled.
Ensure that copying or moving these objects, which likely reference a database connection or internal states, is actually safe in multi-threaded usage. If concurrency is expected, confirm resource alignment or wraps.
132-132: Default private constructor.
Allowing a private default constructor is fine if only the factory is meant to instantiate this class.
149-149: Friend declaration for MySqlStorageFactory.
No concerns; this is consistent with the factory pattern that instantiates these objects.
154-159: Default copy and move ctors for MySqlDataStorage.
Similar to MySqlMetadataStorage, please double-check that copying or moving is safe if these objects hold or reference live connections.
207-207: Default private constructor.
Keeping this constructor private ensures that only the factory can instantiate or manage these objects.
209-209: Friend declaration for MySqlStorageFactory.
This is correct and consistent with preventing direct instantiation outside the factory.
| data = m_data_cache[data_id]; | ||
| } else { | ||
| if (false == m_data_store->get_data(m_conn, data_id, &data).success()) { | ||
| if (false == m_data_store->get_data(*m_conn, data_id, &data).success()) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check pointer validity
All these lines dereference m_conn. If m_conn is ever nullptr, the code risks undefined behaviour. Consider adding defensive checks or ensuring the pointer is never null.
Also applies to: 103-103, 105-105, 117-117, 125-125
| namespace spider::core { | ||
| class MySqlStorageFactory : public StorageFactory { | ||
| public: | ||
| explicit MySqlStorageFactory(std::string const& url); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider URL validation in constructor
The constructor takes a URL string but doesn't appear to validate it. While implementation details aren't shown here, it would be valuable to add validation logic to ensure the URL format is correct before attempting to use it for connections.
🏁 Script executed:
#!/bin/bash
# Look for URL validation in the MySqlStorageFactory implementation
echo "Checking for URL validation in MySqlStorageFactory implementation..."
rg -A 5 "MySqlStorageFactory::MySqlStorageFactory" src/Length of output: 729
Action Required: Incorporate URL Validation in the Constructor
The current implementation of the MySqlStorageFactory constructor directly assigns the URL to m_url without validating its format. To prevent potential connection issues or runtime errors, please add appropriate URL validation logic in the constructor.
- File:
src/spider/storage/mysql/MySqlStorageFactory.cpp - Location: Constructor implementation (initialisation line
: m_url{url})
| return Data{std::move(data), m_data_store, m_storage_factory, m_connection}; | ||
| } |
There was a problem hiding this comment.
Possible bug not returning the new connection.
You return the Data object with the old m_connection pointer, which may remain null if it was never provided, instead of returning the newly created conn. This can cause runtime errors if the new Data instance expects a valid connection.
Below is a sample diff showing how you might pass the newly created connection instead:
- return Data{std::move(data), m_data_store, m_storage_factory, m_connection};
+ return Data{std::move(data), m_data_store, m_storage_factory, conn};📝 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.
| return Data{std::move(data), m_data_store, m_storage_factory, m_connection}; | |
| } | |
| return Data{std::move(data), m_data_store, m_storage_factory, conn}; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/spider/scheduler/scheduler.cpp (1)
214-223: Typo in error message at line 207There's a typo in the error message at line 207: "Failed to connection to storage" should be "Failed to connect to storage" to be consistent with the fixes at lines 88 and 126.
- "Failed to connection to storage: {}", + "Failed to connect to storage: {}",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/spider/CMakeLists.txt(5 hunks)src/spider/client/Driver.cpp(3 hunks)src/spider/client/TaskContext.cpp(3 hunks)src/spider/io/msgpack_message.cpp(4 hunks)src/spider/scheduler/scheduler.cpp(10 hunks)src/spider/storage/DataStorage.hpp(1 hunks)src/spider/storage/MetadataStorage.hpp(1 hunks)src/spider/storage/StorageFactory.hpp(1 hunks)src/spider/storage/mysql/MySqlConnection.cpp(2 hunks)src/spider/storage/mysql/MySqlConnection.hpp(1 hunks)src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp(1 hunks)src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp(3 hunks)src/spider/storage/mysql/MySqlStorage.hpp(4 hunks)src/spider/storage/mysql/MySqlStorageFactory.cpp(1 hunks)src/spider/storage/mysql/MySqlStorageFactory.hpp(1 hunks)src/spider/worker/worker.cpp(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/spider/io/msgpack_message.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/spider/storage/mysql/MySqlConnection.cpp
- src/spider/storage/DataStorage.hpp
- src/spider/client/TaskContext.cpp
- src/spider/storage/mysql/MySqlJobSubmissionBatch.cpp
- src/spider/CMakeLists.txt
- src/spider/storage/MetadataStorage.hpp
- src/spider/storage/StorageFactory.hpp
- src/spider/storage/mysql/MySqlStorageFactory.cpp
- src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/worker/worker.cpp
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/client/Driver.cpp
[performance] 28-28: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/scheduler/scheduler.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (33)
src/spider/storage/mysql/MySqlStorageFactory.hpp (1)
1-32: Implementation looks good but consider URL validationThe
MySqlStorageFactoryclass correctly implements the factory pattern, providing methods to create various storage components. This abstraction will make it easier to switch between storage backends as mentioned in the PR objectives.However, the constructor takes a URL string without validating it. Consider adding validation to ensure URL correctness before using it to establish connections.
You could add validation in the constructor or in a separate method:
explicit MySqlStorageFactory(std::string url) { + // Validate URL format + if (!is_valid_mysql_url(url)) { + throw std::invalid_argument("Invalid MySQL URL format"); + } m_url = std::move(url); }src/spider/storage/mysql/MySqlConnection.hpp (4)
16-17: LGTM: Good use of forward declarationThe forward declaration for
MySqlStorageFactoryis properly added, reducing unnecessary includes.
29-29: LGTM: Proper use of overrideGood use of the
overridekeyword for the destructor, which helps catch errors at compile time if the base class signature changes.
35-36: LGTM: Appropriate return type change for factory patternThe modified signature properly returns a unique pointer to the base
StorageConnectioninterface instead of a concreteMySqlConnectionobject, aligning with the abstract factory pattern.
42-42: LGTM: Appropriate friend declarationThe friend declaration appropriately grants
MySqlStorageFactoryaccess to the private members ofMySqlConnection, which is necessary for the factory pattern implementation.src/spider/client/Driver.cpp (4)
19-20: LGTM: Proper header inclusionThe new includes are appropriate for the storage factory pattern implementation.
25-39: LGTM: Well implemented factory pattern usageThis constructor has been properly updated to use the storage factory pattern. It now obtains storage components through the factory instead of creating them directly, which improves modularity and makes it easier to switch storage backends.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 28-28: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
48-65: LGTM: Properly updated heartbeat threadThe heartbeat thread now correctly uses the storage factory to obtain connections.
68-107: LGTM: Consistent factory pattern implementationThe second constructor is also properly updated to use the storage factory pattern, maintaining consistency with the first constructor.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/scheduler/scheduler.cpp (7)
9-9: LGTM: Added necessary utility headerThe addition of the utility header is appropriate for the changes made in this file.
28-30: LGTM: Updated headers to support factory patternThe new includes correctly support the storage factory pattern implementation.
74-109: LGTM: Well-refactored heartbeat loopThe heartbeat loop function has been properly updated to use the storage factory for obtaining connections. The error message is also improved from "Failed to connection to storage" to "Failed to connect to storage".
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
112-155: LGTM: Well-refactored cleanup loopThe cleanup loop function has been properly updated to use the storage factory for obtaining connections, consistent with the changes in the heartbeat loop.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
195-213: LGTM: Proper storage initializationThe storage initialization code now correctly uses the factory to obtain storage objects, which is consistent with the abstract factory pattern.
247-252: LGTM: Proper thread creationThe heartbeat thread creation now correctly passes the storage factory as a reference, which is consistent with the updated function signature.
256-262: LGTM: Consistent thread creationThe cleanup thread creation is also properly updated to pass the storage factory as a reference, maintaining consistency with the heartbeat thread.
src/spider/worker/worker.cpp (12)
12-12: Good addition of utility header for std::move.Adding the utility header ensures proper access to std::move which is used throughout the code for transferring ownership of unique pointers.
42-44: Well-structured include updates supporting abstract factory pattern.The removed MySQL-specific headers and addition of abstract storage interfaces align with the PR objective to implement the abstract factory pattern. This change decouples the worker from specific storage implementations.
104-106: Properly modified heartbeat_loop signature to use StorageFactory.The function now accepts a shared_ptr to the abstract StorageFactory interface rather than being tightly coupled to MySQL implementation. This is properly passed in the thread creation at lines 463-464.
Also applies to: 463-464
114-126: Good implementation of storage connection acquisition via factory.The code now correctly obtains a connection through the abstract factory pattern and properly handles ownership with std::move. This provides flexibility for different storage backends.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
118-118: Fixed typo in error message.The error message correctly states "Failed to connect to storage" instead of the previous "Failed to connection to storage".
128-128: Proper connection dereferencing from unique_ptr.The code correctly dereferences the connection using *conn when passing it to update_heartbeat, which is consistent with the ownership model change.
226-230: Well-refactored task_loop signature for abstract factory pattern.The function signature now includes the StorageFactory and is properly used in the thread creation. The storage_url parameter is still passed through for TaskExecutor, maintaining compatibility.
Also applies to: 472-475
251-263: Consistent connection management pattern throughout the code.Both code segments use the same pattern for obtaining and managing connections from the storage factory, showing consistent implementation of the abstract factory pattern.
Also applies to: 297-308
264-264: Consistent connection dereferencing throughout task operations.All function calls that previously took a connection reference now correctly use the dereferenced unique_ptr (*conn), maintaining consistent use of the new ownership model.
Also applies to: 274-274, 312-312, 326-326, 341-341, 355-355
418-423: Excellent implementation of the storage factory pattern.This section demonstrates the core of the refactoring - creating a shared factory and using it to provide the necessary storage objects. This approach makes it easier to add new storage backends.
430-442: Consistent connection handling in the main function.The main function follows the same pattern for obtaining and using connections, maintaining consistency throughout the codebase.
452-453: Updated WorkerClient to leverage storage factory.The WorkerClient constructor now accepts the storage factory, which allows it to remain agnostic about the specific storage implementation.
src/spider/storage/mysql/MySqlStorage.hpp (5)
30-31: Good addition of forward declaration for friend class.This forward declaration allows establishing the friend relationship with MySqlStorageFactory without circular dependencies.
33-39: Well-structured class declaration with copy/move support.The MySqlMetadataStorage class now properly supports copy and move operations, which provides more flexibility for the factory pattern implementation and potential use with standard containers.
131-131: Appropriate encapsulation with private constructor and friend factory.Making the default constructor private and befriending MySqlStorageFactory enforces the factory pattern by preventing direct instantiation while allowing the factory to create instances.
Also applies to: 148-148
153-157: Consistent copy/move support for MySqlDataStorage.Similar to MySqlMetadataStorage, MySqlDataStorage now properly supports copy and move operations, maintaining consistency across the storage classes.
206-208: Consistent encapsulation pattern for MySqlDataStorage.The same pattern of private default constructor and friend factory is applied to MySqlDataStorage, maintaining a consistent design pattern throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/storage/StorageTestHelper.hpp (3)
17-17: Consider adding a comment explainingStorageFactoryTypeList.The
StorageFactoryTypeListtuple definition is added but its purpose isn't immediately clear within this file. Consider adding a brief comment explaining its role in the test framework.+// List of supported storage factory types used for parameterized tests using StorageFactoryTypeList = std::tuple<core::MySqlStorageFactory>;
19-23: Good implementation of factory creation with template constraints.The
create_storage_factoryfunction template properly implements the factory pattern returning a base class pointer (StorageFactory) while creating a concrete implementation. Therequiresclause correctly ensures type safety.One minor optimization: consider using
return std::make_unique<T>(get_storage_url<T>());to reuse the URL retrieval function and eliminate duplication.template <class T> requires std::same_as<T, core::MySqlStorageFactory> auto create_storage_factory() -> std::unique_ptr<core::StorageFactory> { - return std::make_unique<T>(cMySqlStorageUrl); + return std::make_unique<T>(get_storage_url<T>()); }
19-29: Prepare for future storage backends.Both template functions currently only support MySQL, but the template structure is ready for extension. When adding new storage backends, you'll need to:
- Add the new factory type to
StorageFactoryTypeList- Extend the
requiresclauses to accept the new type- Implement storage URL retrieval logic for each backend
This demonstrates good forward-thinking in the design.
tests/worker/test-FunctionManager.cpp (1)
69-76: Consider consistent pointer ownership semanticsIn these test cases, you're using
unique_ptrfor storage components, but other test cases useshared_ptr. Consider using consistent ownership semantics across test cases where appropriate.Also applies to: 130-136
tests/storage/test-MetadataStorage.cpp (3)
30-73: Consider mitigating potential flakiness in the heartbeat test
Usingstd::this_thread::sleep_for(std::chrono::seconds(1));in tests may cause intermittent failures under heavy load or slow environments. A mocked clock or a configurable delay might improve stability.
125-258: Consider capturing the result ofsubmit_batchin tests
batch->submit_batch(*conn);is invoked without verifying its outcome. Adding a check helps detect unexpected failures.
260-388: Reduce duplication with the job batch tests
These tests replicate job creation logic from the batch-based approach. Consolidating repeated operations into a helper function could minimize code duplication and simplify maintenance.tests/storage/test-DataStorage.cpp (1)
68-105: Remove unused variable assignment
Storing the result inerris redundant when the subsequentREQUIREstatement re-fetches and checks success. Consider removing the unused variable to simplify the code.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 68-68: syntax error
(syntaxError)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/spider/client/TaskContext.cpp(3 hunks)tests/scheduler/test-SchedulerPolicy.cpp(8 hunks)tests/scheduler/test-SchedulerServer.cpp(4 hunks)tests/storage/StorageTestHelper.hpp(1 hunks)tests/storage/test-DataStorage.cpp(4 hunks)tests/storage/test-MetadataStorage.cpp(15 hunks)tests/worker/test-FunctionManager.cpp(5 hunks)tests/worker/test-TaskExecutor.cpp(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spider/client/TaskContext.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
tests/worker/test-TaskExecutor.cpp
[error] 63-63: syntax error
(syntaxError)
tests/storage/test-DataStorage.cpp
[error] 68-68: syntax error
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
🔇 Additional comments (31)
tests/storage/StorageTestHelper.hpp (4)
7-7: Good addition for string support.Adding the string header is necessary to support the
std::stringtype used forcMySqlStorageUrl. This is a good C++ practice to use the standard string type instead of C-style strings.
10-11: Update to factory pattern includes.The includes have been properly updated to support the new abstract factory pattern. Removing the direct storage implementation includes and replacing them with the factory interfaces aligns with the PR's objective of implementing the abstract factory pattern.
14-15: Improved type and naming specificity.Good change from C-style string to
std::stringand renamed from genericcStorageUrlto more specificcMySqlStorageUrl, clearly indicating this is MySQL-specific. This change provides better type safety and clarity.
25-29: Good abstraction for retrieving the storage URL.The
get_storage_urlfunction template properly abstracts away the specific storage URL, which will make it easier to extend with other storage types in the future. The template constraint ensures type safety.tests/scheduler/test-SchedulerServer.cpp (4)
27-28: Good addition of abstract storage interface headersThese new includes are essential for the storage factory pattern implementation, replacing the MySQL-specific headers and enabling a more flexible storage backend approach.
39-46: Well implemented abstract factory patternThe migration from concrete storage types to the factory pattern is cleanly implemented. The test now uses a generic
StorageFactoryTypeListinstead of specific storage implementations, and properly obtains the storage components through the factory.
48-52: Proper connection handling with variantThe connection handling has been updated to use a variant that can contain either a connection or an error, with appropriate checking before use. Converting from a unique_ptr to a shared_ptr ensures proper ownership semantics.
85-85: Correctly updated method calls with dereferenced connectionThe storage method calls now properly dereference the connection pointer when passing it as an argument, adapting to the new connection management approach.
Also applies to: 100-100
tests/scheduler/test-SchedulerPolicy.cpp (3)
23-24: Good addition of abstract storage interface headersThese new includes support the storage factory pattern implementation, replacing the direct MySQL connection dependency.
31-44: Consistent factory pattern implementation across test casesThe code properly implements the abstract factory pattern across multiple test cases, creating a storage factory and obtaining the necessary storage components through it. The connection handling with variants is implemented consistently.
Also applies to: 96-107, 146-157
55-55: Correctly updated method calls with dereferenced connectionAll storage method calls have been updated to dereference the connection pointer when passing it as an argument, ensuring compatibility with the new storage abstraction.
Also applies to: 63-63, 83-84, 117-118, 138-138, 167-169, 174-175, 185-185
tests/worker/test-TaskExecutor.cpp (4)
26-27: Good addition of abstract storage interface headersThese new includes support the storage factory pattern implementation, providing access to the necessary abstractions.
63-67: Enhanced test flexibility with templated test casesConverting to
TEMPLATE_LIST_TEST_CASEwithStorageFactoryTypeListallows testing with different storage implementations, improving test coverage and flexibility.Also applies to: 95-99, 125-129
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 63-63: syntax error
(syntaxError)
81-81: Updated storage URL retrieval to support multiple storage typesThe storage URL is now retrieved using a template function that supports different storage factory types, rather than using a fixed storage URL.
Also applies to: 113-113, 143-143, 193-193
160-170: Well-implemented factory pattern for storage componentsThe test now properly uses the storage factory to create storage components and correctly handles the connection with variant checking.
tests/worker/test-FunctionManager.cpp (5)
20-23: Good addition of abstract storage interface headersThese new includes support the comprehensive implementation of the storage factory pattern, replacing the direct MySQL dependencies.
82-83: Updated task context creation with storage factoryCorrectly added the storage factory as a parameter to the task context creation, ensuring the context has access to all required storage components.
Also applies to: 141-142
162-172: Well-implemented connection handling with variantThe connection handling correctly uses a variant to manage potential errors, with appropriate checking before use of the connection.
186-187: Properly updated context creation parametersAdded the storage factory parameter to task context creation, ensuring the context has access to all necessary components.
180-181: Correctly updated method calls with dereferenced connectionStorage method calls now properly dereference the connection pointer when passing it as an argument, ensuring compatibility with the new storage abstraction.
Also applies to: 198-198
tests/storage/test-MetadataStorage.cpp (4)
7-7: Adopt new includes for storage abstraction
The newly added includes facilitate the abstract factory design for storage.Also applies to: 21-24
75-124: Scheduler interface changes look consistent
The new factory-based approach integrates well with the scheduler functions. Implementation and checks appear correct.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 75-75: syntax error
(syntaxError)
389-461: Task finishing logic is well integrated
The flow of transitioning task states upon completion matches the intended execution model and ensures accurate test coverage.
462-558: Job reset flow is consistent
Reset behaviour appropriately clears outputs and sets task states as expected. Good coverage across possible task states.tests/storage/test-DataStorage.cpp (7)
2-3: Add memory and utility includes
These headers are essential for the new smart pointer usage and move semantics.
17-20: Add new storage includes
Including the storage interfaces aligns with the abstract factory pattern.
26-66: Test coverage for data creation and retrieval is thorough
The tests accurately verify adding, retrieving, and removing driver data.
109-149: Task key value data test looks correct
The approach for storing and retrieving key-value data for tasks is straightforward and aligns with the factory-based storage system.
154-208: Reference checks for tasks are properly validated
Ensuring a reference cannot be added without existing data or tasks provides robust coverage of edge cases.
212-250: Driver reference approach looks correct
Adding and removing driver references demonstrates good sanity checks for non-existent data or drivers.
68-68: Static analysis syntax error is likely a false positive
No syntactical issues appear at line 68, so this is probably a spurious error from the linter.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 68-68: syntax error
(syntaxError)
1 participant
Description
A valid storage backend (e.g. MySQL) implements multiple interfaces, including
StorageConnection,DataStorage,MetadataStorageandJobSubmissionBatch. Right now the concrete classes are constructed at multiple places, making the abstraction useless as every dependency on the underlying concrete class needs to change when a new backend is added.This pr introduces abstract factory pattern into storage. Each backend implements its own
StorageFactory, which provides the four classes mentioned above. When switching the backend, only the construction of theStorageFactoryobject needs to change, while the rest of code only depends on the interface remain ignorant of the underlying backend.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
StorageFactoryclass to facilitate flexible storage management.StorageConnectionandStorageFactory.