fix: Add storage connection in storage interface - #72
Conversation
WalkthroughThis pull request refactors database connection management across the project. A new header, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component (Worker/Client/Server)
participant M as MySqlConnection
participant S as Storage System
C->>M: MySqlConnection::create(url)
alt Connection success
M-->>C: MySqlConnection (conn)
C->>S: operation(conn, parameters)
S-->>C: Result/Success
else Connection failure
M-->>C: StorageErr
C->>C: Handle error (throw ConnectionException)
end
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (15)
✨ Finishing Touches
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: 3
🧹 Nitpick comments (35)
src/spider/client/Job.hpp (3)
80-80: Verify connection reuse within the loop.The code reuses the same connection object within the polling loop, which is efficient. However, consider adding a timeout or maximum retry count to prevent infinite polling in case of persistent failures.
while (!complete) { constexpr int cSleepMs = 10; + constexpr int cMaxRetries = 1000; // 10 seconds maximum wait time + static int retryCount = 0; + + if (++retryCount > cMaxRetries) { + throw ConnectionException{"Timeout waiting for job completion"}; + } std::this_thread::sleep_for(std::chrono::milliseconds(cSleepMs)); err = m_metadata_storage->get_job_complete(conn, m_id, &complete); if (!err.success()) { throw ConnectionException{ fmt::format("Failed to get job completion status: {}", err.description) }; } }
137-143: Consider extracting connection creation to a helper method.The connection creation pattern is repeated in multiple methods (wait_complete, get_status, get_result). Consider extracting this to a private helper method to reduce code duplication.
+ /** + * Creates a MySQL connection or throws an exception on failure. + * + * @return A reference to the created MySQL connection. + * @throw spider::ConnectionException on connection failure. + */ + private: + auto create_connection() -> core::MySqlConnection& { + std::variant<core::MySqlConnection, core::StorageErr> conn_result + = core::MySqlConnection::create(m_data_storage->get_url()); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description); + } + return std::get<core::MySqlConnection>(conn_result); + }And then simplify the methods:
auto get_result() -> ReturnType { - std::variant<core::MySqlConnection, core::StorageErr> conn_result - = core::MySqlConnection::create(m_data_storage->get_url()); - if (std::holds_alternative<core::StorageErr>(conn_result)) { - throw ConnectionException(std::get<core::StorageErr>(conn_result).description); - } - core::MySqlConnection& conn = std::get<core::MySqlConnection>(conn_result); + core::MySqlConnection& conn = create_connection(); // Rest of the method... }
287-288: Unimplemented get_error method should be updated to match connection pattern.The get_error method is marked as not implemented, but it should follow the same connection pattern as the other methods for consistency when it's eventually implemented.
auto get_error() -> std::pair<std::string, std::string> { - throw ConnectionException{"Not implemented"}; + std::variant<core::MySqlConnection, core::StorageErr> conn_result + = core::MySqlConnection::create(m_data_storage->get_url()); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description); + } + core::MySqlConnection& conn = std::get<core::MySqlConnection>(conn_result); + + // TODO: Implement error retrieval logic using the connection + throw ConnectionException{"Not implemented"}; }tests/worker/test-TaskExecutor.cpp (2)
149-153: Consider extracting connection setup into a reusable utility.
These lines introduce in-test connection creation and validation logic, which appears consistent with the rest of the file. However, factoring out this repeated connection setup into a helper function could improve maintainability if you frequently establish connections in multiple tests.
190-190: Add tests for negative scenarios.
While removing data using an existing connection works here, consider adding negative or edge-case tests (e.g., removing non-existent data) to further validate expected error handling.src/spider/client/TaskContext.cpp (3)
20-26: Centralize error handling for connection failures.
Creating a variant, checking for errors, and throwing aConnectionExceptionis correct. To avoid repeating this pattern, consider encapsulating connection creation and error handling in a dedicated helper function or class. This can simplify your code and ensure consistent exception messages across methods.Also applies to: 28-28
38-45: Replicate the same connection logic with caution.
Again, the connection logic is duplicated for inserting key-value data. Refactoring it into a single code path can prevent potential future mistakes and ensure uniform logging or error handling.Also applies to: 47-47
54-60: Maintain consistent exception details for debugging.
You throw aConnectionExceptionif the result is aStorageErr, which is good for consistency. Ensure your messages are consistent with the other methods for easier debugging, and consider capturing additional context (e.g., which connection URL failed) if necessary.Also applies to: 62-62
src/spider/worker/FunctionManager.hpp (1)
288-296: Extract repeated connection pattern into a helper.
The code correctly creates and checks aMySqlConnectionbefore proceeding, aligning with the new design. However, several methods across the codebase use a nearly identical pattern. Factor out a small utility that handles variant creation and error checks, returning a valid connection or prebuilt error, to reduce duplication.src/spider/client/TaskContext.hpp (2)
155-165: Consider extracting repeated connection-creation logic.
Creating and verifying the MySQL connection is repeated in multiple methods. Factor it out into a helper function or a shared utility to reduce code duplication and maintain consistent error handling.
205-218: Reduce duplication for connection setup.
These lines replicate the same pattern found in the other start method. Extracting a shared helper would simplify the code and ensure consistent connection handling.src/spider/client/Data.hpp (2)
68-75: Centralise connection creation error handling.
This block duplicates the connection-creation pattern found elsewhere. Consider creating a shared function that attempts to establish a connection and returns or throws on failure to maintain consistency.
118-123: Repetitive connection code appears again.
This logic is the same as in set_locality. Consolidating this into a single helper function helps ensure uniform connection handling and error reporting throughout the codebase.src/spider/worker/WorkerClient.cpp (1)
49-49: Typo in error message.There's a grammatical error in the log message.
- "Failed to connection to storage: {}", + "Failed to connect to storage: {}",src/spider/client/Driver.hpp (1)
250-262: Robust connection handling in get_jobs method.The method follows the same pattern as the start methods, with proper connection creation, error checking, and exception throwing. There's one minor inconsistency in the error check where it uses fully qualified
spider::core::StorageErrwhile other methods just usecore::StorageErr.For consistency, consider using unqualified names:
- if (std::holds_alternative<spider::core::StorageErr>(conn_result)) { - throw ConnectionException(std::get<spider::core::StorageErr>(conn_result).description); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description);src/spider/scheduler/SchedulerServer.cpp (1)
163-164: Creating a task instance with an external connection.
Passingm_connhere centralizes connection management. If concurrency is anticipated, ensure proper synchronization or separate connections per thread to avoid data races.src/spider/client/Driver.cpp (3)
29-37: Establishing and verifying a MySQL connection in the constructor.
This approach properly checks for errors before proceeding. However, consider storing the connection object if you plan to reuse it, rather than reconstructing a new connection whenever you need to interact with the database.
64-74: Second constructor duplicates connection logic.
The logic is essentially the same as the previous constructor. To keep code DRY, consider consolidating connection creation and error handling into a common function.
119-125: Structured retrieval of stored values with error handling.
The approach is correct. If performance or concurrency is a concern, consider using a shared connection or a well-managed pool to reduce overhead.Also applies to: 127-127
src/spider/worker/worker.cpp (4)
41-43: New includes for MySQL and storage connections.
These headers are properly introduced. Make sure they are only included where needed to maintain compilation speed.
262-263: Handling argument parsing failure.
task_failis correctly invoked with the established connection. Check that any partial data or side effects are properly cleaned up.
275-275: Storing performance overhead of re-fetching storage URL.
The call tometadata_store->get_url()might be repeated. Consider storing the URL or restricting retrieval calls for performance reasons.
389-405: Connection creation inmainfor adding the driver.
This pattern is consistent with the rest of the code. If the worker frequently restarts, consider re-using or pooling connections.src/spider/scheduler/FifoPolicy.hpp (1)
43-43: Consider adding documentation for the member variableAdding a reference member to store the connection is appropriate. Consider adding a comment to explain the ownership semantics - that this class does not own the connection but maintains a reference to an externally managed one.
+ // Reference to an externally managed storage connection core::StorageConnection& m_conn;tests/scheduler/test-SchedulerPolicy.cpp (1)
40-177: Consider refactoring connection creation into a helper functionThe connection creation code is duplicated across multiple test cases. Consider refactoring this into a helper function to reduce duplication and improve maintainability.
+namespace { +// Helper function to create and validate a MySQL connection +auto create_validated_connection(std::shared_ptr<spider::core::MetadataStorage> const& metadata_store) + -> spider::core::MySqlConnection& +{ + std::variant<spider::core::MySqlConnection, spider::core::StorageErr> conn_result + = spider::core::MySqlConnection::create(metadata_store->get_url()); + REQUIRE(std::holds_alternative<spider::core::MySqlConnection>(conn_result)); + return std::get<spider::core::MySqlConnection>(conn_result); +} +} // namespaceThen in each test case:
- std::variant<spider::core::MySqlConnection, spider::core::StorageErr> conn_result - = spider::core::MySqlConnection::create(metadata_store->get_url()); - REQUIRE(std::holds_alternative<spider::core::MySqlConnection>(conn_result)); - spider::core::MySqlConnection& conn = std::get<spider::core::MySqlConnection>(conn_result); + spider::core::MySqlConnection& conn = create_validated_connection(metadata_store);🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 78-78: syntax error
(syntaxError)
tests/storage/test-DataStorage.cpp (4)
24-28: Consider centralising connection creation in a fixture
You are creating and verifying the connection multiple times in each test block. This duplication makes the tests more verbose. Centralising the setup logic in a fixture or a helper function can simplify the test suite.
62-66: Redundant connection creation repeated throughout tests
Once again, consider using a common setup routine for connection creation.
97-100: Repetitive creation of connection in each test block
Consider extracting this logic into a fixture or a helper function to enforce DRY principles.
191-195: Repeated connection creation in test case
Reflects a consistent approach but consider centralising the pattern in a fixture.src/spider/scheduler/scheduler.cpp (2)
82-105: Frequent reconnection may impact performance
Creating a new MySQL connection within each loop iteration might cause unnecessary overhead. The PR objective mentions keeping an open connection in the scheduler, so consider retaining a persistent connection to adhere to the stated design and boost efficiency.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
114-148: Repeated reconnection in cleanup loop
As in the heartbeat loop, establishing a new connection for each iteration could degrade performance. Consider reusing a persistent connection to meet the PR's connection strategy.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
tests/storage/test-MetadataStorage.cpp (4)
29-33: Connection creation for metadata storage tests
Consider extracting common setup logic to reduce repetition.
76-80: New MySQL connection creation in second test
Again, the approach is consistent but repeated. Consider a fixture-based central approach.
123-126: Connection creation repeated
Same suggestion: centralise this logic to adhere to DRY principles.
247-251: Repetitive connection creation
Again, consider a universal fixture approach for obtaining the connection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
src/spider/CMakeLists.txt(1 hunks)src/spider/client/Data.hpp(3 hunks)src/spider/client/Driver.cpp(5 hunks)src/spider/client/Driver.hpp(3 hunks)src/spider/client/Job.hpp(8 hunks)src/spider/client/TaskContext.cpp(2 hunks)src/spider/client/TaskContext.hpp(2 hunks)src/spider/scheduler/FifoPolicy.cpp(6 hunks)src/spider/scheduler/FifoPolicy.hpp(3 hunks)src/spider/scheduler/SchedulerServer.cpp(4 hunks)src/spider/scheduler/SchedulerServer.hpp(3 hunks)src/spider/scheduler/SchedulerTaskCache.cpp(1 hunks)src/spider/scheduler/SchedulerTaskCache.hpp(4 hunks)src/spider/scheduler/scheduler.cpp(6 hunks)src/spider/storage/DataStorage.hpp(2 hunks)src/spider/storage/MetadataStorage.hpp(2 hunks)src/spider/storage/MySqlConnection.hpp(1 hunks)src/spider/storage/MySqlStorage.hpp(2 hunks)src/spider/storage/StorageConnection.hpp(1 hunks)src/spider/worker/FunctionManager.hpp(4 hunks)src/spider/worker/WorkerClient.cpp(3 hunks)src/spider/worker/worker.cpp(11 hunks)tests/scheduler/test-SchedulerPolicy.cpp(9 hunks)tests/scheduler/test-SchedulerServer.cpp(3 hunks)tests/storage/StorageTestHelper.hpp(1 hunks)tests/storage/test-DataStorage.cpp(7 hunks)tests/storage/test-MetadataStorage.cpp(11 hunks)tests/worker/test-FunctionManager.cpp(2 hunks)tests/worker/test-TaskExecutor.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/spider/storage/StorageConnection.hpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/client/Driver.cpp
[performance] 116-116: Variable 'm_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)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (137)
src/spider/CMakeLists.txt (1)
27-27: Integration of new StorageConnection header looks good.The addition of the new
storage/StorageConnection.hppheader file to theSPIDER_CORE_HEADERSlist is correctly positioned within the storage-related headers section, maintaining a logical organization of the codebase.src/spider/storage/MySqlConnection.hpp (2)
12-12: LGTM!The inclusion of the new StorageConnection header is appropriate for the inheritance relationship being established.
17-17: Inheritance relationship looks good.The change to make MySqlConnection inherit from StorageConnection aligns with the PR objective of improving connection management. This establishes a clear hierarchy in the storage connection classes.
src/spider/scheduler/SchedulerTaskCache.cpp (2)
85-85: Connection parameter correctly passed to get_ready_tasks.This change ensures that the existing storage connection is reused rather than creating a new one for each query, reducing connection overhead as intended by the PR.
90-90: Connection parameter correctly passed to get_task_timeout.Consistent with the other connection-related changes, this modification ensures the scheduler reuses the same connection for all database operations, improving efficiency.
src/spider/client/Job.hpp (9)
63-69: Connection creation and error handling implemented correctly.The implementation properly creates a MySQL connection and handles potential errors by throwing a ConnectionException with a descriptive message. This pattern is consistent with the PR objective of giving control over connection management to the components.
71-71: Connection object correctly passed to get_job_complete method.The modification properly passes the connection object to the metadata storage method, ensuring that the same connection is used throughout the operation.
101-107: Connection creation pattern is consistently applied.The connection creation and error handling pattern is consistently applied across all methods, which is good for maintainability and reliability.
109-109: Connection correctly passed to get_job_status method.The change properly passes the connection object to the metadata storage method.
146-146: Connection correctly passed to get_job_output_tasks.The modification properly passes the connection object to the metadata storage method.
155-155: Connection correctly passed to get_task.The modification properly passes the connection object to the metadata storage method.
186-186: Connection correctly passed to get_data.The modification properly passes the connection object to the data storage method.
227-227: Connection correctly passed to get_task in the alternative result case.The change ensures consistent connection usage throughout both code paths of the get_result method.
245-245: Connection correctly passed to get_data in the alternative result case.The modification ensures consistent connection usage throughout both code paths of the get_result method.
tests/worker/test-TaskExecutor.cpp (1)
161-162: Looks good for verifying successful driver addition.
These lines confirm that the driver is added with a valid connection, aligning with the new persistent connection approach. The code is clean and straightforward.src/spider/worker/FunctionManager.hpp (2)
13-13: Header inclusion is appropriate.
Including<variant>is valid if you rely onstd::variantusage, as seen further down. This is a good addition.
29-30: Validate necessity of new includes.
BothMySqlConnection.hppandStorageConnection.hppare included to support the revised connection logic. This seems correct, though confirm thatStorageConnection.hppis explicitly needed.src/spider/scheduler/SchedulerTaskCache.hpp (2)
18-18: New header include is valid.
No concerns regarding this addition.
27-27: Verify the lifetime of the referenced connection.
Storing a reference tocore::StorageConnectioncan lead to undefined behaviour if the connection goes out of scope before theSchedulerTaskCacheis destroyed. Confirm that the connection object consistently outlives this class.Also applies to: 36-36, 52-52
src/spider/client/Data.hpp (1)
17-17: Header import looks appropriate.
No issues observed with introducingMySqlConnection.hpp.src/spider/worker/WorkerClient.cpp (3)
15-15: Added necessary header for logging.The addition of the spdlog header is appropriate to support the new error logging functionality.
24-25: Added required headers for storage connection handling.These headers are essential for the new connection management approach, aligning with the PR's objective to manage connections explicitly.
55-55: Properly passing connection to storage method.Using an explicit connection object rather than creating one internally is in line with the PR goal of explicit connection management, helping to reduce connection overhead.
tests/worker/test-FunctionManager.cpp (3)
151-155: Properly implemented explicit connection management in test.The connection is now properly created and validated before use. The REQUIRE statement ensures the test fails if connection creation fails, which is good test practice.
162-163: Using the established connection for database operations.The test now correctly passes the explicit connection to the storage methods, ensuring consistent connection usage.
179-179: Consistently using the connection for cleanup operations.The change ensures the same connection is used for cleanup, which is a good practice for maintaining transactional consistency.
src/spider/client/Driver.hpp (2)
176-185: Proper connection handling with comprehensive error reporting.The implementation correctly:
- Creates an explicit connection
- Checks for connection errors
- Throws a meaningful exception with the error description if connection fails
- Uses the connection for the storage operation
This aligns perfectly with the PR objective to manage connections explicitly.
225-235: Consistent connection handling in graph-based start method.The implementation mirrors the pattern used in the task-based start method, ensuring consistent behavior across both variants. The explicit connection creation and error handling is well implemented.
src/spider/scheduler/SchedulerServer.hpp (3)
11-11: Added required header for explicit connection management.The StorageConnection header inclusion is necessary for the new connection reference parameter and member variable.
31-31: Added explicit connection parameter to constructor.This change allows callers to pass an existing connection to the SchedulerServer, enabling connection reuse and reducing connection overhead, which is a key goal of this PR.
49-49: Storing connection as a reference member.Using a reference rather than a new connection instance is appropriate here since:
- It avoids creating unnecessary connections
- It allows the scheduler to maintain an open, long-lived connection as mentioned in the PR description
- It ensures the storage operations use the same connection
The approach aligns with the PR objective of allowing components to decide whether to maintain an open connection.
src/spider/scheduler/SchedulerServer.cpp (4)
22-22: Include usage is correct.
This new include forStorageConnection.hppaligns with the added reference to a storage connection in the constructor.
34-41: Constructor now stores a reference to an existing connection.
This change enables the scheduler to reuse an existing storage connection rather than creating a new one. Ensure other components calling this constructor can provide a valid, open connection and handle any connection lifecycle issues.
142-143: Validate error handling when retrieving the job ID.
Usingm_connto fetch the job ID is consistent with the new design. Confirm that the caller properly logs or propagates errors if this step fails, especially since the job ID is crucial for resetting the job.
151-151: Explicitly reset job with the shared connection.
Callingreset_jobwithm_connis straightforward. Verify that this operation does not conflict with other concurrent operations that might rely on the same connection.src/spider/client/Driver.cpp (1)
105-111: Handling connection creation before inserting key-value data.
This robustly validates that you have a working connection for your KV store operation. Validate thatkv_store_insertusage is thoroughly tested for concurrent calls.tests/storage/StorageTestHelper.hpp (3)
28-32: Creating data storage with a verified connection.
Good practice verifying the connection variant before initializing the storage. Ensure that test code also handles failures (e.g., DB offline) gracefully to avoid test flakiness.
40-44: Creating metadata storage with a verified connection.
Similarly, this cleanly checks forcore::MySqlConnectionbefore callinginitialize(conn). Confirm that any storage initialization errors are clearly reported in test logs.
52-55: Initializing both metadata and data storage in a single call.
The approach minimizes redundancy. If additional storage types are introduced, consider consolidating the repeated connection logic in a helper method to keep code DRY.Also applies to: 58-58, 60-60
src/spider/worker/worker.cpp (4)
12-12: Adding<variant>for efficient error handling.
Including<variant>is consistent with the rest of the changes that returnstd::variant<...>for connection results.
234-244: Connection retry logic withintask_loop.
Constructing a fresh connection on every loop iteration functions as a fallback. Beware of the potential overhead if frequent loops and fails occur.
251-251: Retrieving a task with a single shared connection.
Invokingget_taskusingconnis consistent with the approach. Validate that no concurrency issues arise if multiple tasks run in parallel.
287-288: Marking tasks as failing upon executor error.
task_failis appropriately invoked when the executor fails. Ensure that subsequent logic (such as re-trying tasks) is well-defined.src/spider/scheduler/FifoPolicy.hpp (2)
15-15: Appropriate addition of the StorageConnection headerAdding the StorageConnection header is necessary to support the new connection parameter in the class.
24-29: Constructor signature change aligns with explicit connection managementThe modification to add a
StorageConnectionparameter aligns with the PR objective to explicitly manage storage connections. This allows the scheduler to maintain an open connection rather than creating new ones for each operation.src/spider/scheduler/FifoPolicy.cpp (6)
24-29: Updated function signature to include connection parameterAdding the connection parameter to the
task_locality_satisfiedfunction is appropriate and consistent with the broader objective of explicit connection management.
40-40: Connection parameter properly passed to storage methodThe connection is correctly passed to the
get_datamethod, which is consistent with the PR's objective of explicit connection management.
63-75: Constructor implementation aligns with header changesThe constructor properly initializes the connection member variable and passes it to the task cache. This implementation is consistent with the changes in the header file.
88-88: Verify function call includes connection parameterThe connection is correctly passed to the
task_locality_satisfiedfunction, maintaining consistency with the updated function signature.
106-107: Storage operation now uses explicit connectionThe
get_task_job_idmethod now correctly receives the connection parameter, which is consistent with the PR's objective.
123-125: Storage operation includes connection parameterThe
get_job_metadatamethod now correctly receives the connection parameter, which is consistent with the PR's objective of explicit connection management.tests/scheduler/test-SchedulerServer.cpp (5)
48-52: Connection creation and validation implemented correctlyThe code correctly creates a MySQL connection and validates that it was created successfully before proceeding. This ensures that tests don't continue with an invalid connection.
54-54: Policy initialization includes connection parameterThe
FifoPolicyinitialization correctly includes the connection parameter, which is consistent with the changes to its constructor.
59-59: Server initialization includes connection parameterThe
SchedulerServerinitialization correctly includes the connection parameter, which is consistent with the changes to its constructor.
84-84: Add job operation includes connection parameterThe
add_jobmethod call correctly includes the connection parameter, which is consistent with the changes to the storage interface.
99-99: Remove job operation includes connection parameterThe
remove_jobmethod call correctly includes the connection parameter, which is consistent with the changes to the storage interface.tests/scheduler/test-SchedulerPolicy.cpp (7)
40-44: Connection creation and validation in first test caseThe code correctly creates and validates a MySQL connection for the first test case. This ensures that the test uses a valid connection for all storage operations.
54-54: Storage operations include connection parameterThe
add_joboperations correctly include the connection parameter, which is consistent with the changes to the storage interface.Also applies to: 62-62
64-64: Policy initialization includes connection parameterThe
FifoPolicyinitialization correctly includes the connection parameter, consistent with the changes to its constructor.
74-75: Remove job operations include connection parameterThe
remove_joboperations correctly include the connection parameter, which is consistent with the changes to the storage interface.
93-97: Connection creation for second test case follows patternThe connection creation and validation for the second test case follows the same pattern as the first test case, maintaining consistency across tests.
106-108: Driver and data operations include connection parameterThe
add_driverandadd_driver_dataoperations correctly include the connection parameter, which is consistent with the changes to the storage interface.
145-149: Connection creation pattern maintained in third test caseThe connection creation and validation for the third test case maintains the same pattern as the previous test cases, ensuring consistency throughout the test file.
tests/storage/test-DataStorage.cpp (27)
33-34: Good job on explicit connection usage
These lines maintain clarity by requiring an explicit connection for database operations. This approach is consistent with the PR objective.
39-39: DuplicateKeyErr check is correct
This check ensures that attempting to insert an existing key returns the proper error type.
43-43: Successful data retrieval behaviour validated
Ensuring that the data is properly retrieved from the database is crucial for correctness. This line has no issues.
47-47: Data removal test coverage is solid
The operation is validated to ensure the data gets removed successfully.
51-51: Key not found error check is correct
Confirming that a removed record triggers the right error is beneficial for reliability.
70-70: Explicit add_driver usage with connection parameter is consistent
Explicitly providing the connection object aligns with the new design.
74-74: Ensuring add_client_kv_data honours the new signature
This line correctly calls the method with an explicit connection.
79-79: DuplicateKeyErr check is present for client kv data
Ensuring that duplicates are handled gracefully.
83-84: Comprehensive retrieval test for client kv data
Validates that we can retrieve the data successfully using the explicit connection.
109-109: Explicit job addition with connection object
Helps unify the approach of requiring a valid connection.
113-113: Add task KV data with explicit connection
Adhering to the new approach ensures consistent usage of the MySQL connection.
118-118: Check for DuplicateKeyErr
This coverage ensures robust handling of conflicting task KV data.
122-122: Retrieving task KV data
Verifying that the method returns correct data using the established connection.
126-126: Remove job with explicit connection
This ensures the job cleanup path is properly tested.
144-144: Expecting a fail on invalid task reference creation
This test properly confirms that an invalid reference scenario returns a failure.
164-164: Proper usage of add_task_data with explicit connection
Maintains consistency with the new database connection approach.
166-166: Verify adding a valid task reference
Ensures that the reference linking function operates successfully.
169-169: Check proper removal of task reference
This ensures correct clean-up of references in the DB.
172-172: Job removal at test completion
Good practice to ensure test data cleanup.
175-175: Verification of removing dangling data
Ensures leftover references are cleaned up properly.
180-180: Confirm KeyNotFoundErr after removing data references
The final check that the data was fully removed from storage.
199-199: Verifying failure on referencing unknown driver and data
The test ensures the method returns failure for invalid references.
204-205: Multiple driver registration
Verifies that the metadata storage successfully handles multiple driver additions using the new connection approach.
208-208: Ensuring references to non-existent data fails
Validates robust error handling for driver references.
212-212: Storing driver data
This test ensures that the new data is properly stored for an existing driver.
215-215: Adding valid driver reference
Verifies that referencing a valid driver and data succeeds.
218-218: Ensuring driver reference removal
Properly tests the removal of references for the second driver.src/spider/scheduler/scheduler.cpp (5)
191-203: Connection creation logic in main
Establishes the primary MySQL connection and ensures we have a valid object. No immediate issues detected.
204-204: Initialising the metadata storage with an explicit connection
Follows the new design requirement to pass a valid connection for initialisation.
209-209: Initialising the data storage
Using the same connection ensures consistent usage of the MySQL connection across storages.
222-224: Unified approach to passing connection to the scheduler policy and server
This code is consistent with simplifying connection management in the main function.
228-228: Scheduler registration with explicit connection
Ensures that adding a scheduler record uses the shared connection.tests/storage/test-MetadataStorage.cpp (34)
39-39: Driver addition with explicit connection
Ensures the operation is tested with the correct connection usage.
43-43: Heartbeat timeout check
Verifies the driver does not time out prematurely. Implementation is correct.
52-52: Heartbeat timeout for driver
Ensures that the driver eventually times out as expected.
60-60: Updating heartbeat
Explicitly passing the connection confirms usage in the new design.
62-62: Final heartbeat timeout check
Confirms the driver is no longer timed out after the heartbeat update.
86-87: Scheduler addition with explicit connection
This coverage is crucial for verifying the new approach in scheduler creation.
92-92: Retrieving the scheduler address
Ensures the underlying DB call returns correct data.
98-98: Checking scheduler retrieval for a non-existent ID
Verifies we get the appropriate KeyNotFoundErr for missing scheduler.
102-102: Retrieving default scheduler state
Valid test to confirm default state is "normal."
108-108: Setting scheduler state
Ensures that we can update the scheduler's state with the explicit connection.
111-111: Confirming new scheduler state
This test verifies retrieval of the updated "recovery" state.
178-179: Adding multiple jobs with explicit connections
Verifies creation of multiple jobs under the same client ID.
183-183: Retrieving jobs by client ID
Ensures the DB call returns an empty list for unknown client ID.
187-187: Retrieving existing jobs by client ID
Tests correct retrieval of the newly added job IDs.
196-197: Retrieving job metadata
Successfully verifies job metadata retrieval using the explicit connection.
205-206: Fetching the task graph
Confirms the underlying structure is retrieved accurately.
208-209: Retrieving the simple_graph
Validates that the simpler job's graph is fetched correctly.
213-214: Individual task retrieval
Verifies fetching a single task from DB with the new connection approach.
218-219: Retrieving child tasks
Ensures the DB properly returns all children of a specified parent.
224-225: Retrieving parent tasks
Checks that the multi-parent scenario is handled properly by the DB.
234-235: Removing job
Ensures that the job is deleted from storage as expected.
236-237: Confirm KeyNotFoundErr for removed job
Once removed, subsequent fetch attempts must fail accordingly.
238-239: Re-checking the remaining job
Ensures the other added job is still accessible.
240-241: Final job removal
Cleans up the last job.
279-280: Adding job with explicit connection
Adheres to the updated design requiring an open connection.
283-285: Setting task state and finishing task
Verifies that tasks can transition to Running state and properly finish with outputs.
360-363: Task finish with explicit connection
Supports verifying partial completion in the job. The approach properly records output data.
366-368: Parent task state transition and finish
Ensures the second parent transitions correctly to Running and finishes with the provided output.
372-374: Completing the child task
Demonstrates that the child transitions from Pending to Running to Finished with the given outputs.
378-379: Testing job reset
Validates that the entire job returns to its initial state with cleared outputs.
384-384: Verify retrieving the reset parent's task
Confirms the parent's state and inputs are properly reverted.
390-390: Output clearance after reset
Ensures that the parent's output is cleared upon job reset.
398-398: Verifying child task reversion
Checks the child's state post-reset is back to Pending without any outputs.
406-406: Final job removal
Ensures test data is cleaned up.src/spider/storage/DataStorage.hpp (2)
11-11: Include directive looks good.The addition of “StorageConnection.hpp” clearly indicates that the interface now depends on a shared connection context. No concerns here.
23-78: Ensure proper documentation of the newStorageConnection¶meter.All these interface methods are now requiring a connection reference. This is a beneficial change for explicit resource management, but it would be helpful to document the intended usage and lifetime of the
StorageConnectionobject in Doxygen comments or similar. This helps avoid possible confusion for future maintainers.
[verify, architecture_advice]src/spider/storage/MetadataStorage.hpp (2)
15-15: Include directive is correct.Adding “StorageConnection.hpp” aligns with the shift toward explicit connection usage. No issues identified.
27-130: All methods now require aStorageConnection&: verify correct usage and lifetime.By requiring the
StorageConnection¶meter, the design clarifies connection management and prevents accidental creation or reuse errors. Be sure to validate that each caller properly manages the connection’s lifetime, especially in multi-threaded scenarios.
[verify, architecture_advice]src/spider/storage/MySqlStorage.hpp (4)
24-24: Good addition of the new include.Pulling in “StorageConnection.hpp” is consistent with the new connection-oriented design.
30-33: Constructor usage updated successfully.These changes provide an explicit constructor for
MySqlMetadataStorageand remove unnecessary semicolons. The approach is correct and improves clarity.
38-122: NewStorageConnection&in methods enhances explicit resource usage.Enforcing a valid connection reference across all metadata methods is a positive step towards robust error handling and connection reuse. Confirm concurrency safety if multiple threads share the same connection.
135-190:MySqlDataStoragefollows the same connection-based pattern.Similar to
MySqlMetadataStorage, ensuring theconnreference remains valid during method execution is crucial. Document the concurrency approach if the same connection is expected for parallel calls.
| std::variant<core::MySqlConnection, core::StorageErr> conn_result | ||
| = core::MySqlConnection::create(m_metadata_storage->get_url()); | ||
| if (std::holds_alternative<core::StorageErr>(conn_result)) { | ||
| throw ConnectionException(std::get<core::StorageErr>(conn_result).description); | ||
| } | ||
| core::MySqlConnection& conn = std::get<core::MySqlConnection>(conn_result); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heartbeat thread re-creates the connection each time.
Repeatedly building the connection in this loop may cause performance overhead. Consider reusing a single connection or employing a connection pool mechanism with reconnection logic if needed.
| std::variant<spider::core::MySqlConnection, spider::core::StorageErr> conn_result | ||
| = spider::core::MySqlConnection::create(metadata_store->get_url()); | ||
| if (std::holds_alternative<spider::core::StorageErr>(conn_result)) { | ||
| spdlog::error( | ||
| "Failed to connection to storage: {}", | ||
| std::get<spider::core::StorageErr>(conn_result).description | ||
| ); | ||
| fail_count++; | ||
| continue; | ||
| } | ||
| spider::core::MySqlConnection& conn = std::get<spider::core::MySqlConnection>(conn_result); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heartbeat loop with repeated connection attempts.
Making a new connection for every heartbeat can degrade performance. Consider a connection pool or a persistent connection approach with retry logic to reduce overhead.
Also applies to: 124-125
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/spider/worker/WorkerClient.cpp (1)
45-55:⚠️ Potential issuePotential crash in error handling logic.
The current implementation logs the error but doesn't return or throw an exception when the connection fails. If there's a connection error, line 53 will attempt to get a
MySqlConnectionfrom a variant containing aStorageErr, leading to astd::bad_variant_accessexception.Apply this fix:
if (std::holds_alternative<spider::core::StorageErr>(conn_result)) { spdlog::error( "Failed to connection to storage: {}", std::get<spider::core::StorageErr>(conn_result).description ); + return std::nullopt; } spider::core::MySqlConnection& conn = std::get<spider::core::MySqlConnection>(conn_result);
🧹 Nitpick comments (2)
src/spider/worker/WorkerClient.cpp (2)
49-49: Fix typo in error message.There's a small typo in the error message: "Failed to connection to storage" should be "Failed to connect to storage".
- "Failed to connection to storage: {}", + "Failed to connect to storage: {}",
45-55: Design suggestion: Consider connection pooling for the worker.Since the PR objectives mention high-frequency operations causing overhead with new connections, consider implementing connection pooling for the worker client as well. This would allow reusing connections across multiple calls to
get_next_taskrather than creating a new connection each time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/worker/WorkerClient.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (3)
src/spider/worker/WorkerClient.cpp (3)
15-15: Include for logging functionality added correctly.The addition of the spdlog header is appropriate for the new error logging in the connection handling code.
24-25: Storage connection headers added correctly.The additions of MySqlConnection and StorageConnection headers are required for the new connection management implementation.
45-55:Details
✅ Verification successful
Consider connection cleanup.
While the code properly creates and uses a database connection, it doesn't explicitly close or release the connection when it's no longer needed. The connection will eventually be closed when the
MySqlConnectionobject goes out of scope, but explicitly managing the connection lifecycle might be clearer.Verify that
MySqlConnectionproperly handles cleanup in its destructor, or consider adding explicit cleanup code.
🏁 Script executed:
#!/bin/bash # Look for connection cleanup in MySqlConnection destructor rg -A 10 "MySqlConnection::~MySqlConnection" src/Length of output: 751
Verified Connection Cleanup in Destructor
The review comment suggesting explicit connection cleanup can be resolved. The destructor for
MySqlConnection(located insrc/spider/storage/MySqlConnection.cpp) already handles cleanup by closing the connection and resetting the pointer. No additional cleanup code is required in theWorkerClientimplementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
src/spider/worker/WorkerClient.cpp (1)
46-56: Good error handling for database connections, but return early instead of using nullopt.The connection handling logic is well-implemented with proper error logging and robust error handling. However, by returning
std::nulloptwhen a connection error occurs, you're making it difficult to distinguish between a connection error and the absence of tasks. Consider returning a more expressive result or using a dedicated error type.Adding a more descriptive error message or using an error code would make debugging easier:
if (std::holds_alternative<spider::core::StorageErr>(conn_result)) { spdlog::error( "Failed to connect to storage: {}", std::get<spider::core::StorageErr>(conn_result).description ); - return std::nullopt; + return std::nullopt; // Connection error }src/spider/client/Driver.cpp (2)
86-101: Duplicate heartbeat thread logic in second constructor.The heartbeat thread logic is duplicated in both constructors. Consider refactoring to eliminate this duplication by extracting the heartbeat thread creation into a separate private method.
Extract the heartbeat logic into a private method:
+private: + void start_heartbeat_thread() { + // Start a thread to send heartbeats + // NOLINTNEXTLINE(performance-unnecessary-value-param) + m_heartbeat_thread = std::jthread([this](std::stop_token stoken) { + std::variant<core::MySqlConnection, core::StorageErr> conn_result + = core::MySqlConnection::create(m_metadata_storage->get_url()); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description); + } + auto& conn = std::get<core::MySqlConnection>(conn_result); + + while (!stoken.stop_requested()) { + std::this_thread::sleep_for(std::chrono::seconds(1)); + core::StorageErr const err = m_metadata_storage->update_heartbeat(conn, m_id); + if (!err.success()) { + throw ConnectionException(err.description); + } + } + }); + }Then call this method from both constructors instead of duplicating the code.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
116-116: Variable 'm_id' is assigned in constructor body instead of initialization list.Consider performing initialization in the initialization list for consistency and potentially better performance.
Driver::Driver(std::string const& storage_url) { - boost::uuids::random_generator gen; - m_id = gen(); + : m_id(boost::uuids::random_generator()()) { m_metadata_storage = std::make_shared<core::MySqlMetadataStorage>(storage_url);🧰 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/FifoPolicy.hpp (1)
42-45: Document the purpose of the connection member.While the code correctly adds the connection member, adding a brief comment explaining why the connection is stored as a member (likely for performance reasons) would be helpful for future maintainers.
std::shared_ptr<core::MetadataStorage> m_metadata_store; std::shared_ptr<core::DataStorage> m_data_store; // NOLINTNEXTLINE(cppcoreguidelines-avoid-const-or-ref-data-members) + // Stored as a member to reuse the same connection for multiple operations core::StorageConnection& m_conn;src/spider/worker/worker.cpp (3)
114-117: Typographical nitpick
Change the message"Failed to connection to storage: {}"to"Failed to connect to storage: {}"for clarity and correctness.-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
236-238: Typographical nitpick
Change"Failed to connection to storage: {}"to"Failed to connect to storage: {}".-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);
387-399: Grammar improvement
Change"Failed to connection to storage: {}"to"Failed to connect to storage: {}"for accuracy.-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);src/spider/scheduler/scheduler.cpp (3)
85-86: Typographical nitpick
Change"Failed to connection to storage: {}"to"Failed to connect to storage: {}".-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);
119-121: Typographical nitpick
Change the message to"Failed to connect to storage: {}".-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);
193-202: Typographical nitpick
Again, please update"Failed to connection to storage: {}"to"Failed to connect to storage: {}"to maintain consistency.-spdlog::error( - "Failed to connection to storage: {}", - std::get<spider::core::StorageErr>(conn_result).description -); +spdlog::error( + "Failed to connect to storage: {}", + std::get<spider::core::StorageErr>(conn_result).description +);src/spider/storage/MetadataStorage.hpp (1)
48-130: Ensure concurrency safety with reference-based connections.
PassingStorageConnection&for all operations centralizes connection management. However, if these methods are called from multiple threads, watch for concurrency concerns—particularly if the underlying connection object is not thread-safe. You may need synchronization or a separate connection per thread if concurrency is critical.src/spider/storage/MySqlStorage.hpp (2)
30-31: Constructor enforcement of connection context.
By deleting the default constructor and providing a single constructor that accepts a URL, you ensure that each instance ofMySqlMetadataStoragehas a specified storage location. This design complements the newly mandatedStorageConnection&in subsequent methods but might be slightly more restrictive for mocking or testing if URL-based instantiation is unnecessary.Would you like a secondary constructor for advanced testing scenarios?
135-189: Promote consistent connection handling inMySqlDataStorage.
Similar to the metadata class, passingStorageConnection&ensures explicit connection control. Confirm that all data-related transaction boundaries and error conditions (e.g., partial write failures) are handled appropriately. If the expected usage patterns differ for data storage, consider documenting best practices so consumers understand how to manage the connection lifecycle (e.g., whether they should hold it open persistently).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/spider/client/Data.hpp(4 hunks)src/spider/client/Driver.cpp(7 hunks)src/spider/client/Driver.hpp(5 hunks)src/spider/client/Job.hpp(10 hunks)src/spider/client/TaskContext.cpp(3 hunks)src/spider/client/TaskContext.hpp(4 hunks)src/spider/scheduler/FifoPolicy.cpp(6 hunks)src/spider/scheduler/FifoPolicy.hpp(3 hunks)src/spider/scheduler/scheduler.cpp(6 hunks)src/spider/storage/DataStorage.hpp(2 hunks)src/spider/storage/MetadataStorage.hpp(2 hunks)src/spider/storage/MySqlStorage.hpp(2 hunks)src/spider/worker/FunctionManager.hpp(4 hunks)src/spider/worker/WorkerClient.cpp(2 hunks)src/spider/worker/worker.cpp(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/spider/client/Driver.hpp
- src/spider/client/TaskContext.hpp
- src/spider/client/Job.hpp
- src/spider/client/TaskContext.cpp
- src/spider/scheduler/FifoPolicy.cpp
- src/spider/worker/FunctionManager.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/scheduler/scheduler.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] 116-116: Variable 'm_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: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (30)
src/spider/worker/WorkerClient.cpp (2)
12-12: Well-organized includes: Clear separation of standard, Boost, and project-specific headers.The includes are properly organized and grouped, which improves readability. You've added
<variant>,<spdlog/spdlog.h>,Error.hpp, andMySqlConnection.hppto support the new connection handling functionality.Also applies to: 16-16, 19-19, 26-26
48-54: Potential crash in error handling logic has been properly addressed.I see that you've addressed the previous issue raised about potential crashes. Now you correctly check for a connection error before attempting to access the
MySqlConnection, and returnstd::nulloptwhen a connection error occurs.src/spider/client/Data.hpp (4)
9-9: Well-organized include directives with new dependencies.The include directives are properly organized, with the new
<variant>andMySqlConnection.hppheaders added to support the connection management functionality.Also applies to: 18-18
69-76: Good connection error handling with proper exception.The connection error handling in
set_localitymethod is well-implemented, using aConnectionExceptionwith a meaningful error message when a connection fails. The connection is appropriately passed to the storage operation.
119-125: Consistent connection handling in the Builder class.The same connection handling pattern is consistently applied in the
buildmethod, ensuring that connection errors are properly handled throughout the codebase.
128-139: Connection now properly passed to storage methods.The connection is now explicitly passed to
add_driver_dataandadd_task_datamethods, ensuring that all storage operations use the established connection.src/spider/client/Driver.cpp (4)
9-9: Proper inclusion of required headers.The necessary headers
<variant>and../storage/MySqlConnection.hpphave been added to support the new connection management functionality.Also applies to: 18-18
31-39: Good connection error handling in the constructor.The connection error handling in the constructor is well-implemented, throwing a
ConnectionExceptionwith a meaningful error message when a connection fails. The connection is appropriately passed to the storage operation.
48-63: 🛠️ Refactor suggestionHeartbeat thread re-creates the connection each time.
Repeatedly building the connection in this loop may cause performance overhead. Consider reusing a single connection or employing a connection pool mechanism with reconnection logic if needed.
Consider refactoring to reuse the connection:
m_heartbeat_thread = std::jthread([this](std::stop_token stoken) { + std::variant<core::MySqlConnection, core::StorageErr> conn_result + = core::MySqlConnection::create(m_metadata_storage->get_url()); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description); + } + auto& conn = std::get<core::MySqlConnection>(conn_result); + while (!stoken.stop_requested()) { std::this_thread::sleep_for(std::chrono::seconds(1)); - std::variant<core::MySqlConnection, core::StorageErr> conn_result - = core::MySqlConnection::create(m_metadata_storage->get_url()); - if (std::holds_alternative<core::StorageErr>(conn_result)) { - throw ConnectionException(std::get<core::StorageErr>(conn_result).description); - } - auto& conn = std::get<core::MySqlConnection>(conn_result); - core::StorageErr const err = m_metadata_storage->update_heartbeat(conn, m_id); if (!err.success()) { - throw ConnectionException(err.description); + // Try to reconnect once before throwing an exception + conn_result = core::MySqlConnection::create(m_metadata_storage->get_url()); + if (std::holds_alternative<core::StorageErr>(conn_result)) { + throw ConnectionException(std::get<core::StorageErr>(conn_result).description); + } + conn = std::get<core::MySqlConnection>(conn_result); + + // Try the update again with the new connection + core::StorageErr const retry_err = m_metadata_storage->update_heartbeat(conn, m_id); + if (!retry_err.success()) { + throw ConnectionException(retry_err.description); + } } } });
107-114: Consistent connection management in KV store methods.The KV store methods consistently apply the same connection handling pattern, ensuring that connection errors are properly handled throughout the codebase.
src/spider/scheduler/FifoPolicy.hpp (3)
15-15: Adding the StorageConnection header in the appropriate location.The StorageConnection header is properly included with other storage-related headers, maintaining good code organization.
24-29: Constructor signature updated to include connection parameter.The constructor has been properly updated to include a
core::StorageConnection¶meter, which is a good approach for explicit connection management.
43-44: Connection member variable properly added with appropriate NOLINT comment.The
m_connmember variable has been correctly added as a reference with an appropriate NOLINT comment to suppress the linter warning about const or reference data members.src/spider/worker/worker.cpp (4)
12-12: No issues found
Including<variant>is appropriate and introduces no immediate concerns.
41-41: No issues found
Including theMySqlConnectionheader aligns with the revised design for explicit connections.
111-135: Repeated connection creation may degrade performance
As previously noted, creating a newMySqlConnectioninside thewhileloop can degrade performance. Consider a persistent connection approach or connection pool to avoid establishing a connection each iteration.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
233-310: Repeated connection creation in task loop
Similar to the heartbeat loop, establishing a new connection each time can be costly. Using a shared or pooled connection might improve performance, especially under heavy load.src/spider/scheduler/scheduler.cpp (7)
9-9: No issues found
Adding<variant>here is in line with the connection-result design.
81-105: Repeated connection creation in heartbeat loop
CreatingMySqlConnectionwithin each iteration could adversely affect performance. A persistent connection or a retry-based approach can mitigate overhead.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
116-147: Repeated connection creation in cleanup loop
Like the heartbeat loop, repeatedly creating connections here may hinder system efficiency under heavy load. A pooled or persistent connection strategy is recommended.🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
203-210: No issues found
The storage initialisation logic is in alignment with the new connection-oriented design.
221-221: No issues found
InvokingFifoPolicywith an existing connection is consistent with the new architecture.
223-223: No issues found
Constructing theSchedulerServerwith the shared connection is coherent with the updated design.
227-227: No issues found
Passing the connection toadd_schedulerensures consistency with the design.src/spider/storage/DataStorage.hpp (1)
11-78: Ensure concurrency and lifetime correctness
Enforcing the new signature withStorageConnection& connin many methods integrates well with the connection-oriented design. However, confirm that concurrency scenarios handle shared connections correctly. Retaining references in multi-threaded environments can cause undefined behaviour if connections close prematurely.src/spider/storage/MetadataStorage.hpp (3)
15-15: Include guard consistency check.
Including"StorageConnection.hpp"here is consistent with the newly introduced parameter in method signatures. Ensure that corresponding include guards and forward declarations in"StorageConnection.hpp"do not conflict with existing ones to avoid potential redefinition issues.
27-46: Potential breaking API change due to connection parameter.
All methods now require aStorageConnection& connparameter. This alteration breaks downstream code unless all call sites are updated to pass an appropriateStorageConnectionreference. Confirm that all implementers of this interface and all callers have been adjusted accordingly, and ensure thorough documentation reflecting the new usage requirement.
131-131: Retain return-by-reference pattern carefully.
Using[[nodiscard]] auto get_url() const -> std::string const&might reduce copies, but ensure the lifetime ofm_urlis guaranteed, especially if subclassers store or alter it. In multi-threaded usage, be cautious about concurrent reads and writes.src/spider/storage/MySqlStorage.hpp (2)
24-24: Necessary inclusion of the unified connection interface.
#include "StorageConnection.hpp"aligns with the new design. Validate that no cyclical dependencies exist with the newly introduced interface. This is essential for clean compilation and maintainability.
38-121: Method signature updates withStorageConnection&inMySqlMetadataStorage.
All metadata operations now rely on an external connection, which promotes clarity in resource usage. Double-check that each method handles connection failures—e.g., disconnected or invalid states—consistently. If an exception or error code is raised for an invalid connection, ensure the caller has a clear path to handle or re-establish it.
1 participant
Description
Previously, all components create new storage connections on need to save database connection resource, as stated in #57. However, this induce significant overhead, especially in scheduler, which is performance critical and execute storage queries frequently.
This pr adds storage connection explicitly in the storage interface and leaves it to the user of the storage interface to decide whether to keep a connection open or create a new one on need. Scheduler takes the first approach, while other components remain using the later.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit