feat: Add batch job submission - #75
Conversation
WalkthroughThis update reorganises MySQL-related files by moving source and header files into a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant D as Driver
participant B as JobSubmissionBatch
participant M as MetadataStorage
participant S as MySQL Storage
C->>D: begin_batch_start()
D->>B: Initialize batch (MySqlJobSubmissionBatch)
loop For each job
C->>D: start(job)
D->>B: Add job to batch
end
C->>D: end_batch_start()
D->>M: add_job_batch(conn, batch, job_id, client_id, task_graph)
M->>S: Execute batch submission via prepared statements
S-->>M: Return StorageErr/Success
M-->>D: Relay submission result
D-->>C: Batch submission complete
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 2
🔭 Outside diff range comments (1)
src/spider/storage/mysql/MySqlStorage.cpp (1)
496-622: 🛠️ Refactor suggestionImplements
add_job_batchto handle large graphs in one shot.
This is crucial for efficient multi‐job insertion. The logic closely parallelsadd_job, so watch for any mismatch in ordering or rollback scenarios.
🧹 Nitpick comments (11)
tests/client/client-test.cpp (1)
160-177: Added test case for batch job submission.This new test validates the batch job submission functionality introduced in the PR. It demonstrates how to:
- Begin a batch submission with
begin_batch_start()- Submit multiple jobs in a batch
- End the batch with
end_batch_start()- Verify the correct execution of all jobs in the batch
This test ensures the new feature works as intended and provides an example of its usage.
I notice that error conditions in the batch job test are logged but don't result in a return code that would indicate test failure. Consider returning the error code to ensure test failures are properly detected:
if (job.get_status() != spider::JobStatus::Succeeded) { spdlog::error("Batch job failed"); + return cJobFailed; } int const result = job.get_result(); if (result != i + i) { spdlog::error("Batch job wrong result. Expect {}. Get {}.", i + i, result); + return cJobFailed; }src/spider/client/Driver.cpp (1)
25-27: Consider initializing m_id in the initialization listThe m_id member is assigned in the constructor body. Consider moving it to the initialization list for consistency with C++ best practices.
-Driver::Driver(std::string const& storage_url) { - boost::uuids::random_generator gen; - m_id = gen(); +Driver::Driver(std::string const& storage_url) + : m_id(boost::uuids::random_generator{}()) {🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 27-27: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/client/Driver.hpp (2)
138-171: Provides straightforward batch submission control.
Thebegin_batch_start()andend_batch_start()methods neatly encapsulate batch mode logic, preventing multiple concurrent batches. Be mindful of the scenario wherebegin_batch_start()is called butend_batch_start()is never invoked, which may leave an openm_batch. You might want to ensure or document that users always finalize their batches.
296-297: Introduces storage connection and batch member variables.
Storing these asstd::shared_ptris consistent with the new workflow. Just be mindful that ifm_connorm_batchbecome invalid externally, references here might lead to undefined behaviour.src/spider/storage/mysql/MySqlStorage.hpp (2)
52-58: Introducesadd_job_batchto the interface.
This method will allow clients to submit multiple tasks within a single transaction, aligning with the batch submission design. Ensure thorough integration tests to confirm consistent database state.
136-147: Adds overloading helpers for task insertion.
Splitting betweenadd_taskandadd_task_batchhelps avoid duplicated logic. Consider factoring out shared code if duplication grows.src/spider/storage/mysql/MySqlStorage.cpp (1)
221-300: Implementsadd_taskto insert tasks with optional states.
This handles function name, task inputs, and outputs thoroughly. Be sure to exercise complete coverage in unit tests, as partial inserts in error scenarios can be tricky.src/spider/client/Job.hpp (3)
94-105: Check for repeated connection creation logic.
Similar fallback logic appears here, which may be extracted into a shared helper to avoid code duplication.
144-155:get_error()not implemented.
Currently, it throws aConnectionExceptionwith a "Not implemented" message. If partial or placeholder error data is available, consider returning some minimal diagnostic info.Do you want me to open a new issue to track implementing the logic for retrieving error details?
157-173: Constructor overloading form_conn.
The two constructors differ only by whether aStorageConnectionis provided. Document this carefully so that future usage is clear—particularly, which scenario each constructor is intended for.src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (1)
41-60: Transactional integrity upon partial failures.
Therollbackin line 54 handles an exception in the submit sequence, which is good. Consider whether partial results linger in memory or if certain data structures also need reset if the transaction fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
src/spider/CMakeLists.txt(2 hunks)src/spider/client/Data.hpp(1 hunks)src/spider/client/Driver.cpp(5 hunks)src/spider/client/Driver.hpp(6 hunks)src/spider/client/Job.hpp(5 hunks)src/spider/client/TaskContext.cpp(1 hunks)src/spider/client/TaskContext.hpp(1 hunks)src/spider/scheduler/scheduler.cpp(1 hunks)src/spider/storage/JobSubmissionBatch.hpp(1 hunks)src/spider/storage/MetadataStorage.hpp(2 hunks)src/spider/storage/mysql/MySqlConnection.cpp(1 hunks)src/spider/storage/mysql/MySqlConnection.hpp(1 hunks)src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp(1 hunks)src/spider/storage/mysql/MySqlStorage.cpp(16 hunks)src/spider/storage/mysql/MySqlStorage.hpp(4 hunks)src/spider/storage/mysql/mysql_stmt.hpp(1 hunks)src/spider/worker/FunctionManager.hpp(1 hunks)src/spider/worker/WorkerClient.cpp(1 hunks)src/spider/worker/task_executor.cpp(1 hunks)src/spider/worker/worker.cpp(1 hunks)tests/client/client-test.cpp(2 hunks)tests/scheduler/test-SchedulerPolicy.cpp(1 hunks)tests/scheduler/test-SchedulerServer.cpp(1 hunks)tests/storage/StorageTestHelper.hpp(1 hunks)tests/storage/test-DataStorage.cpp(1 hunks)tests/storage/test-MetadataStorage.cpp(2 hunks)tests/worker/test-FunctionManager.cpp(1 hunks)tests/worker/test-TaskExecutor.cpp(1 hunks)
🧰 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)
⏰ 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 (49)
src/spider/worker/WorkerClient.cpp (1)
26-26: Include path updated correctly.The path to
MySqlConnection.hpphas been updated to reflect the new directory structure with MySQL-related files now in a dedicatedmysqlsubdirectory. This change is consistent with the project's reorganization of MySQL components.src/spider/client/Data.hpp (1)
18-18: Include path properly updated.The include directive for
MySqlConnection.hpphas been correctly updated to use the new path structure with MySQL components now in a dedicated subdirectory.tests/scheduler/test-SchedulerServer.cpp (1)
28-28: Include path correctly modified.The path to
MySqlConnection.hpphas been properly updated to reflect the new directory structure, maintaining the correct relative path from the test file location.src/spider/client/TaskContext.cpp (1)
12-12: Include path properly updated.The path to
MySqlConnection.hpphas been correctly modified to accommodate the new directory structure with MySQL components in a dedicated subdirectory.src/spider/storage/mysql/MySqlConnection.hpp (1)
11-12: Include paths properly adjusted after file relocation.Since
MySqlConnection.hpphas been moved to themysqlsubdirectory, the relative paths to the included headers have been correctly updated:
- Path to
Error.hppnow goes up two directory levels instead of one- Path to
StorageConnection.hppnow goes up one level since it remains in the parent directoryThese changes maintain proper include resolution after the file reorganization.
tests/storage/test-DataStorage.cpp (1)
16-16: Update include path for MySqlConnection header.The include directive has been updated to point to the new location in the
mysqlsubdirectory, which is consistent with the repository reorganisation.tests/scheduler/test-SchedulerPolicy.cpp (1)
24-24: Revised MySqlConnection include path.The include directive now correctly reflects the new
mysqlsubdirectory structure. This update is consistent with the overall restructuring of MySQL-related files.tests/worker/test-TaskExecutor.cpp (1)
25-25: Correct MySqlConnection include update.The include path has been updated to use the new location in
mysql, which aligns with the repository’s updated directory structure.src/spider/worker/task_executor.cpp (1)
27-27: Update include for MySqlStorage header.The change to include
"../storage/mysql/MySqlStorage.hpp"instead of the former path is correct and in line with the new file organisation. There is no functional impact from this.tests/worker/test-FunctionManager.cpp (1)
20-20: Adjust include directive for MySqlConnection.The include directive now points to the
mysqlsubdirectory version ofMySqlConnection.hpp, which is consistent with the project-wide restructuring.src/spider/client/TaskContext.hpp (1)
22-22: Path update is correct.The include path has been correctly updated to reflect the new directory structure for MySQL-related files, which have been organized into a dedicated subdirectory.
src/spider/worker/FunctionManager.hpp (1)
29-29: Path update is correct.The include path has been correctly updated to reflect the new directory structure where MySQL-related files have been moved to a dedicated subdirectory.
src/spider/storage/JobSubmissionBatch.hpp (1)
1-22: Well-designed abstract base class for batch job submission.This new abstract interface provides the foundation for the batch job submission feature. The class is appropriately designed with:
- A pure virtual method for batch submission
- Proper rule-of-five implementation (deleted copy/move operations)
- Virtual destructor for polymorphic inheritance
This design aligns well with the PR objective to support batch job submissions.
tests/storage/StorageTestHelper.hpp (1)
16-17: Path updates are correct.The include paths have been correctly updated to reflect the restructured location of MySQL-related files, which have been moved to a dedicated subdirectory.
src/spider/worker/worker.cpp (1)
41-42: Include paths updated correctly.The MySQL-related header files have been moved to a dedicated
mysqlsubdirectory, consistent with the reorganization mentioned in the PR objectives.src/spider/scheduler/scheduler.cpp (1)
27-28: Include paths updated correctly.The MySQL-related header files have been moved to a dedicated
mysqlsubdirectory, consistent with the reorganization mentioned in the PR objectives.src/spider/storage/MetadataStorage.hpp (2)
15-15: New include for batch job submission functionality.The
JobSubmissionBatch.hppheader file has been added to support the new batch job submission feature.
42-48:Details
✅ Verification successful
New virtual method for batch job submission.
This new method extends the metadata storage interface to support batch job submissions, which aligns with the PR objectives to enhance the Spider application for submitting multiple jobs simultaneously. The method signature is consistent with the existing
add_jobmethod but takes an additionalJobSubmissionBatchparameter.
🏁 Script executed:
#!/bin/bash # Check if the MySqlMetadataStorage implementation includes the add_job_batch method grep -A 10 "add_job_batch" src/spider/storage/mysql/MySqlStorage.hppLength of output: 573
Batch Job Submission Method Verified – Approved
The new virtual method
add_job_batchinsrc/spider/storage/MetadataStorage.hppis correctly implemented inMySqlStorage.hpp(as verified by the grep output). The method signature is consistent with the existingadd_jobfunction and suitably extends support for batch job submissions. No further changes are required.tests/client/client-test.cpp (1)
78-78: Variable renamed for clarity.The variable
jobhas been renamed tograph_jobto better reflect its purpose and distinguish it from other job types in the test.Also applies to: 80-81, 82-82, 87-88
src/spider/storage/mysql/MySqlConnection.cpp (2)
10-12: Appropriate header changes to support batch processingThe addition of DriverManager and Properties headers provides the necessary components for the batch job submission feature. The DriverManager offers a more robust way to manage database connections while Properties allows configuration of bulk statements.
20-28: Good simplification of connection handlingThe connection establishment has been nicely simplified by:
- Using a Properties object with
useBulkStmts=trueto enable batch operations- Consolidating the connection creation into a single line using DriverManager
- Clarifying the comment to focus on validation rather than parsing
This is a clean improvement that will support the batch job submission functionality while reducing code complexity.
src/spider/CMakeLists.txt (2)
3-4: Well-organized MySQL source file restructuringThe MySQL-related source files have been properly moved to a dedicated
mysqlsubdirectory, which improves code organization by grouping related components together.
28-32: Good addition of batch job submission headersThe CMakeLists.txt correctly includes the new headers necessary for batch job submission:
mysql_stmt.hppfor SQL statement definitionsMySqlJobSubmissionBatch.hppfor batch-specific implementationJobSubmissionBatch.hppfor the interfaceThis structure provides a clean separation between the generic batch submission interface and the MySQL-specific implementation, following good design principles.
src/spider/client/Driver.cpp (2)
37-39: Good use of shared_ptr for connection managementUsing std::make_shared to create and store the MySQL connection is a good improvement. This maintains a single connection for the lifetime of the Driver, which aligns with the PR objective of reducing latency during batch operations by keeping an open storage connection.
112-120: Consistent use of stored connectionMethods now consistently use the stored shared connection pointer (*m_conn) instead of creating temporary connections, which will reduce connection overhead especially when submitting multiple jobs in batch.
🧰 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 (3)
21-22: Appropriate header updates for batch testingThe test file correctly includes the new MySqlJobSubmissionBatch header needed to test the batch submission functionality.
130-184: Good batch processing setup in existing testThe existing "Job add, get and remove" test has been modified to use the new batch processing capabilities. This ensures that the basic job operations work correctly within a batch context.
248-374: Comprehensive test coverage for batch job operationsThe new "Job batch add, get and remove" test case thoroughly validates the batch job submission functionality. It provides complete test coverage by:
- Creating a batch processing context
- Testing complex task graphs with multiple dependencies
- Verifying correct job submission through batch interface
- Confirming job metadata can be retrieved correctly
- Validating task retrieval and relationship handling
- Testing job removal within the batch context
This test case ensures the batch job submission feature works as expected and maintains all the functionality of the non-batch approach.
src/spider/client/Driver.hpp (4)
20-23: Adds necessary includes for new batch submission functionality.
These headers are appropriate for the new batch job submission logic and storage connection handling.
214-228: Conditional logic cleanly ties into batch process.
Usingadd_job_batchwhenm_batchis non-null looks correct. This ensures that multiple job submissions are effectively combined, whereas standalone submissions continue to calladd_job.
266-272: Extendsstart()to handle job creation consistently.
This line correctly usesm_metadata_storage->add_job()if there is no active batch. Ensure that all error codes returned byadd_joboradd_job_batchare properly handled to avoid partial job states in the database.
285-285: Fetches jobs by client ID with the new connection pointer.
Double-check whether any concurrency or race conditions might arise when multiple clients query the same records concurrently. This call otherwise seems correct.src/spider/storage/mysql/MySqlStorage.hpp (2)
5-5: Includes<optional>for optional task states and related logic.
This is a good choice to accommodate the new approach.
15-27: Reorganized includes for MySQL storage files.
The new includes forData.hpp,Driver.hpp, etc., appear consistent with the MySQL–specific classes.src/spider/storage/mysql/MySqlStorage.cpp (5)
30-39: Introduces new headers for extended MySQL job submission.
These imports ensure the new batch submission code is integrated, though watch for potential compilation overhead from many includes.
127-139: Initializes storage with consolidated statements array.
Usingmysql::cCreateStoragein a loop is a neat approach for schema initialization. Verify that partial table creation does not leave an inconsistent state if an error occurs mid-creation.
301-371: Adds batched version of task insertion (add_task_batch).
Your usage of prepared statements and batching can significantly cut overhead. Double-check edge cases, e.g., adding tasks with no inputs or outputs.
374-494: Extendsadd_jobto rollback on any reference inconsistency.
The BFS‐like approach for adding tasks in topological order is logical. If referencing a large graph, ensure performance remains reasonable.
1783-1795: Initializes data storage with the same approach.
Reusingmysql::cCreateStorageagain is consistent. Validate partial creation or concurrency concerns if multiple storage initializations run concurrently.src/spider/storage/mysql/mysql_stmt.hpp (2)
1-206:Details
❓ Verification inconclusive
Ensure that foreign key constraints align with all data lifecycles.
The code extensively usesON DELETE CASCADEand other constraints. While beneficial for integrity, cascaded deletions might remove data that other parts of the system still need. Confirm that this behaviour is fully intended and tested, particularly for thejobs,tasks, anddatatables.You may want to run integration tests that validate whether child records are properly retained or removed after parent records are deleted:
🏁 Script executed:
#!/bin/bash # Pseudocode: Re-run existing integration tests, in case specialized constraints are tested there echo "Please run your integration tests to confirm that no data is inadvertently being deleted."Length of output: 188
Action Required: Verify Foreign Key Cascade Behaviour Thoroughly
The integration test pseudocode you ran only confirmed that an instruction to execute tests exists—it didn’t provide actual test outcomes. Please ensure that your full integration test suite validates the cascading deletion behaviours for thejobs,tasks, anddatatables. Confirm that deletions on parent records do not inadvertently remove child records needed elsewhere in the system.
1-206:Details
❓ Verification inconclusive
Consider additional indexing to optimize query performance.
Several tables, for exampletasksanddata, contain fields (e.g.,state,value) that may be frequently queried or filtered on. Adding secondary indexes or more granular indexing could improve performance when handling large volumes of data.As a follow-up, please verify whether existing queries or usage patterns necessitate additional indexes by examining typical query patterns. You can start by searching for SQL statements referencing these columns:
🏁 Script executed:
#!/bin/bash rg -A 2 -i 'SELECT' | grep -iE 'tasks|data'Length of output: 6673
Action: Reconsider composite indexing on tasks based on query patterns
The query analysis confirms that many operations filter or sort by the state and job_id columns in the tasks table (e.g.,
SELECT state FROM tasks WHERE job_id = ?and other similar queries). In contrast, most queries on the data table primarily use its primary key, so additional indexing there seems less critical. I recommend:
- Evaluating the benefit of a composite index on
(job_id, state)for the tasks table to improve performance on queries filtering by both.- Checking typical workload patterns to ensure that the cost of additional indexes is justified.
Please verify these suggestions against your query execution plans and expected data volumes before implementing.
src/spider/client/Job.hpp (4)
24-25: No issues with the new includes.
These includes look compatible with the updated directory organization.
132-142: Confirm performance when retrieving larger result sets.
Creating a new connection on-demand can introduce overhead if the function is invoked frequently in large-scale scenarios. If tasks or results are large, consider leveraging a persistent connection pattern.
194-325: Comprehensive retrieval logic.
The approach for assembling tuples of outputs and verifying type consistency is quite thorough. Ensure that robust unit tests exist to confirm all branches (e.g., mismatched types, missing data, multiple output tasks).
332-332: Connection pointer usage.
m_connis a shared pointer. Confirm that no cyclic references exist. IfJobcan outlive the connection context, it might inadvertently keep the connection alive.src/spider/storage/mysql/MySqlJobSubmissionBatch.hpp (4)
19-29: EnsureprepareStatementis available on construction.
If this constructor is called too early (e.g., before the database session is fully established), subsequent statements might fail. Confirm that everything is safe at object construction time.
30-39: Consistent usage of theMySqlConnectionpointer.
Both constructors perform nearly identical operations with different parameters. Confirm that the logic is consistent betweensql::Connection&andMySqlConnection&so that prepared statements cannot diverge unexpectedly.
62-80: Getter methods facilitate better reusability.
Joining numerous statements into one class with getters fosters code clarity. This pattern is consistent with the rest of the design.
83-92: Clean resource management withunique_ptr.
Usingstd::unique_ptrforPreparedStatementensures a neat release of resources. This is an appropriate approach for RAII.
| auto wait_complete_conn(core::StorageConnection& conn) -> void { | ||
| bool complete = false; | ||
| core::StorageErr 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) | ||
| }; | ||
| } | ||
| while (!complete) { | ||
| constexpr int cSleepMs = 10; | ||
| 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) | ||
| }; | ||
| } | ||
| } | ||
| auto& conn = std::get<core::MySqlConnection>(conn_result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential infinite loop risk.
The while (!complete) poll/sleep approach may risk indefinite looping if the job is never marked complete. Consider adding a reasonable timeout or max retry count if your system can end up stuck.
| auto wait_complete() -> void { | ||
| 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); | ||
| } | ||
| auto& conn = std::get<core::MySqlConnection>(conn_result); | ||
|
|
||
| bool complete = false; | ||
| core::StorageErr 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) | ||
| }; | ||
| } | ||
| while (!complete) { | ||
| constexpr int cSleepMs = 10; | ||
| 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) | ||
| }; | ||
| if (nullptr == m_conn) { | ||
| 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); | ||
| } | ||
| auto& conn = std::get<core::MySqlConnection>(conn_result); | ||
| wait_complete_conn(conn); | ||
| } | ||
| wait_complete_conn(*m_conn); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate fallback connection creation in wait_complete.
When m_conn is null, the code creates a new MySqlConnection once and then uses wait_complete_conn(conn). However, line 76 unconditionally calls wait_complete_conn(*m_conn) again afterwards. There is no else block to skip that final invocation when a new connection was just created and used. This might result in logically redundant calls or potential misuse if m_conn remains null.
auto wait_complete() -> void {
if (nullptr == m_conn) {
// ...
wait_complete_conn(conn);
- }
- wait_complete_conn(*m_conn);
+ } else {
+ wait_complete_conn(*m_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.
| auto wait_complete() -> void { | |
| 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); | |
| } | |
| auto& conn = std::get<core::MySqlConnection>(conn_result); | |
| bool complete = false; | |
| core::StorageErr 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) | |
| }; | |
| } | |
| while (!complete) { | |
| constexpr int cSleepMs = 10; | |
| 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) | |
| }; | |
| if (nullptr == m_conn) { | |
| 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); | |
| } | |
| auto& conn = std::get<core::MySqlConnection>(conn_result); | |
| wait_complete_conn(conn); | |
| } | |
| wait_complete_conn(*m_conn); | |
| } | |
| auto wait_complete() -> void { | |
| if (nullptr == m_conn) { | |
| 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); | |
| } | |
| auto& conn = std::get<core::MySqlConnection>(conn_result); | |
| wait_complete_conn(conn); | |
| } else { | |
| wait_complete_conn(*m_conn); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/spider/client/Job.hpp (1)
195-327: Consider breaking down the complexget_result_connmethod.The
get_result_connmethod has been marked to suppress complexity warnings, but it remains quite complex and difficult to maintain. Consider breaking it down into smaller, more focused methods that handle specific aspects like type checking, data retrieval, and tuple handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/client/Job.hpp(5 hunks)
⏰ 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 (5)
src/spider/client/Job.hpp (5)
24-25: Updated include path for MySQL components.The include path changes reflect the reorganization of MySQL-related files into a dedicated subdirectory, making the code structure more modular and organized.
66-77: Connection handling improvement with clear conditional logic.The connection handling has been improved by adding a proper conditional check for an existing connection. This fixes the issue from the previous review where
wait_complete_conn(*m_conn)was called unconditionally after creating a new connection.
183-192: Potential infinite loop risk.The
while (!complete)poll/sleep approach still carries a risk of indefinite looping if the job is never marked complete. Consider adding a reasonable timeout or max retry count.while (!complete) { constexpr int cSleepMs = 10; + static constexpr int cMaxRetries = 600; // 6 seconds max wait time + static int retries = 0; 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) }; } + if (++retries >= cMaxRetries) { + throw ConnectionException{"Timed out waiting for job to complete"}; + } }
166-173: Good design for connection reuse in batch operations.The new constructor accepting a shared
StorageConnectionpointer enables the batch job submission functionality described in the PR objectives. This allows for connection reuse across multiple operations, which should reduce latency.
333-333: Good addition of shared connection member.Adding a shared pointer to a storage connection as a member variable is central to enabling the batch processing functionality described in the PR objectives. This allows the job to maintain and reuse an open storage connection.
| auto get_error() -> std::pair<std::string, std::string> { | ||
| throw ConnectionException{"Not implemented"}; | ||
| } |
There was a problem hiding this comment.
Implement the get_error method.
The get_error method is declared but not implemented, throwing a "Not implemented" exception. This is inconsistent with having it as part of the public API. Either implement the method or mark it as a TODO with a clear timeline for implementation.
1 participant
Description
Right now
Spideronly supports submitting one job at a time, which is inefficient when user submit a large group of jobs at the same time. This pr introduces to calls to the driver interface,begin_batch_startandend_batch_start, which can be used around multiplestartcalls, to send all storage requests in batch.Client now also keeps an open storage connection to avoid unnecessary latency. However,
TaskContextin task executor does not keep open storage connection to avoid running out open connection resource.Checklist
breaking change.
Validation performed
Summary by CodeRabbit