feat: delete pipeline #4156
feat: delete pipeline #4156
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughThe recent updates involve various modules concerning pipeline management functionality. This includes changes to function signatures, restructuring imports, refactoring error handling, and adding new data handling methods and structures. Tests were also updated to reflect these changes, including new methods for managing pipelines via HTTP endpoints, and improved clarity in comments. The Changes
Sequence Diagram(s)Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (16)
- src/frontend/src/instance/log_handler.rs (2 hunks)
- src/frontend/src/server.rs (2 hunks)
- src/pipeline/Cargo.toml (1 hunks)
- src/pipeline/src/lib.rs (1 hunks)
- src/pipeline/src/manager.rs (1 hunks)
- src/pipeline/src/manager/error.rs (2 hunks)
- src/pipeline/src/manager/pipeline_operator.rs (5 hunks)
- src/pipeline/src/manager/table.rs (8 hunks)
- src/pipeline/src/manager/util.rs (1 hunks)
- src/servers/src/error.rs (2 hunks)
- src/servers/src/http.rs (3 hunks)
- src/servers/src/http/event.rs (8 hunks)
- src/servers/src/query_handler.rs (2 hunks)
- tests-integration/Cargo.toml (1 hunks)
- tests-integration/src/test_util.rs (1 hunks)
- tests-integration/tests/http.rs (3 hunks)
Files skipped from review due to trivial changes (2)
- src/pipeline/Cargo.toml
- tests-integration/Cargo.toml
Additional comments not posted (20)
src/pipeline/src/lib.rs (1)
21-24: Approved new exports for pipeline management.The added exports are coherent with the described functionality enhancements in pipeline management. Ensure that these new exports are covered by adequate unit tests to maintain code quality.
src/pipeline/src/manager.rs (4)
28-28: Constant for pipeline table name defined.Ensure that the
PIPELINE_TABLE_NAMEis consistent with the actual table names used in the database schema.
30-34: Clear and well-documented type alias forPipelineVersion.The use of
Option<TimestampNanosecond>is appropriate for representing optional data, and the comments add useful context for developers.
36-37: Type alias forPipelineInfois appropriate.This alias correctly encapsulates the necessary data for pipeline information, aligning with the system's requirements.
39-40: Type aliases usingArcfor thread-safe sharing.Ensure that the use of
Arcdoes not introduce memory leaks or unnecessary overhead in the system.src/pipeline/src/manager/util.rs (3)
26-35: Functionto_pipeline_versioncorrectly implemented.The error handling using
InvalidPipelineVersionSnafuis appropriate. Consider adding more comprehensive tests to cover various error scenarios and edge cases.
37-50: Functionbuild_plan_filtereffectively constructs query filters.Ensure that this function integrates smoothly with the rest of the database querying mechanisms, and consider adding integration tests to verify its effectiveness.
52-61: Cache key generation functiongenerate_pipeline_cache_keyis well-implemented.Verify that the cache keys generated by this function are effectively utilized by the caching system and do not lead to performance bottlenecks.
src/frontend/src/instance/log_handler.rs (1)
71-80: Methoddelete_pipelineupdated to handle pipeline versions.The update to include the
versionparameter is crucial for managing different versions of pipelines. Ensure that integration tests cover scenarios with different versions to validate the method's functionality.src/pipeline/src/manager/error.rs (1)
105-110: New error typeInvalidPipelineVersionappropriately defined.Ensure that this error type is handled consistently across the system, particularly in functions that parse or manipulate pipeline versions.
src/servers/src/query_handler.rs (1)
147-152: Approved: Updateddelete_pipelinefunction signature to include versioning.This change aligns with the pipeline management enhancements and is crucial for handling specific pipeline versions. Ensure that all integrations with this function are updated to pass the correct version.
Verification successful
Ensure all integrations with the
delete_pipelinefunction are updated to pass the correct version parameter.The following instances were found:
src/servers/src/query_handler.rssrc/servers/src/http.rssrc/servers/src/http/event.rssrc/pipeline/src/manager/table.rssrc/pipeline/src/manager/pipeline_operator.rssrc/frontend/src/instance/log_handler.rsWe need to verify that each of these calls includes the
versionparameter. Let's inspect these occurrences in detail to confirm the correct usage.
Approved: Updated
delete_pipelinefunction signature to include versioning.The function calls to
delete_pipelineacross the codebase have been verified to include theversionparameter, ensuring proper integration with the updated function signature.
src/servers/src/http/event.rs: Correct usage found.src/pipeline/src/manager/pipeline_operator.rs: Correct usage found.src/frontend/src/instance/log_handler.rs: Correct usage found.No issues found with the integration of the updated function signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `delete_pipeline` in the project. # Test: Search for the function usage. Expect: Only occurances with version parameter. rg --type rust $'delete_pipeline'Length of output: 754
Script:
#!/bin/bash # Description: Verify correct usage of the delete_pipeline function in each file. # Check each occurrence of delete_pipeline for correct parameters for file in src/servers/src/query_handler.rs src/servers/src/http.rs src/servers/src/http/event.rs src/pipeline/src/manager/table.rs src/pipeline/src/manager/pipeline_operator.rs src/frontend/src/instance/log_handler.rs; do echo "Checking $file" rg --context 5 'delete_pipeline' $file doneLength of output: 4905
src/servers/src/http/event.rs (1)
Line range hint
103-135: Approved: Updated HTTP handlers for pipeline operations with version handling.The changes to add and delete pipeline handlers include version handling and validation, which are crucial for accurate pipeline management. Ensure comprehensive testing of these handlers to verify their functionality and error handling.
Also applies to: 147-176
src/servers/src/error.rs (1)
Line range hint
1-250: RemovedUnsupportedDeletePipelineerror variant.The removal of the
UnsupportedDeletePipelineerror variant aligns with the PR's objective to handle pipeline deletions differently. Ensure that all references to this error have been updated throughout the codebase to handle the new logic appropriately.Verification successful
Verification successful: The
UnsupportedDeletePipelineerror variant has been removed, and there are no remaining references to it in the codebase.
- No occurrences of
UnsupportedDeletePipelinewere found in any files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `UnsupportedDeletePipeline` error variant is not used elsewhere in the codebase. # Test: Search for the removed error variant. Expect: No occurrences. rg --type rust --glob '!src/servers/src/error.rs' 'UnsupportedDeletePipeline'Length of output: 77
Script:
#!/bin/bash # Description: Verify that the `UnsupportedDeletePipeline` error variant is not used anywhere in the codebase. # Test: Search for the removed error variant across all files. Expect: No occurrences. rg 'UnsupportedDeletePipeline'Length of output: 30
tests-integration/src/test_util.rs (1)
429-429: Integration ofwith_log_ingest_handlermethod.The addition of the
with_log_ingest_handlermethod in thesetup_test_http_app_with_frontend_and_user_providerfunction is crucial for handling log ingestion in tests. Ensure that the integration is properly tested and that the method behaves as expected under various scenarios.tests-integration/tests/http.rs (5)
1033-1041: Verify successful pipeline creation.The test creates a pipeline and checks for a successful response. Ensure that the content of the response is also validated to confirm that the pipeline was created correctly, not just that the server responded with
StatusCode::OK.Consider adding assertions to validate the response content more deeply, such as checking specific fields in the JSON response.
1090-1097: Test failure behavior after pipeline deletion.This test checks that writing data to a deleted pipeline fails. It is a good practice to verify the type of error returned to ensure it is meaningful and guides the user correctly.
Consider asserting the specific error message or error code returned to ensure it provides clear feedback to the user.
1003-1005: Ensure initialization logging is appropriate for test context.The line
common_telemetry::init_default_ut_logging();initializes logging for unit tests. Confirm that this is appropriate for integration testing and not just unit testing.
1052-1057: Validate pipeline version handling.The test extracts and uses the pipeline version from the response. It's crucial to ensure that the version handling is robust, especially given the URL encoding in line 1079.
Verification successful
Pipeline version handling appears robust.
The test suite consistently retrieves, checks, converts, and URL encodes the pipeline version before using it in requests. This ensures that the version handling is robust.
tests-integration/tests/http.rs:
- Lines 1052-1057: Retrieval and validation of the version.
- Line 1079: URL encoding of the version.
- Line 1081: Usage of the encoded version in a DELETE request.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent handling of pipeline version across the test suite. rg --type rust 'version.as_bytes()' tests-integration/Length of output: 182
Script:
#!/bin/bash # Description: Check for consistent handling of the encoded pipeline version in tests-integration/tests/http.rs rg --context 5 'url::form_urlencoded::byte_serialize(version.as_bytes()).collect()' tests-integration/tests/http.rsLength of output: 117
Script:
#!/bin/bash # Description: Search for all instances of "version" in tests-integration/tests/http.rs to verify its handling. rg 'version' tests-integration/tests/http.rsLength of output: 571
1081-1088: Ensure proper cleanup after pipeline deletion.The test deletes the pipeline and checks for a successful response. It is important to verify that the pipeline is actually removed from the system, not just that the deletion request was successful.
src/servers/src/http.rs (1)
593-601: Refactor suggestion forwith_log_ingest_handlermethod.This method correctly adds routing for log ingestion. However, consider using consistent error handling and middleware application as seen in other similar methods in this file to ensure uniform behavior across all routes.
[REFACTOR_SUGGESTion]- self.router.nest(&format!("/{HTTP_API_VERSION}/events"), HttpServer::route_log(handler, validator)), + self.router.nest(&format!("/{HTTP_API_VERSION}/events"), HttpServer::route_log(handler, validator).layer(common_middleware()))
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4156 +/- ##
==========================================
+ Coverage 84.95% 85.04% +0.08%
==========================================
Files 1061 1063 +2
Lines 188105 188339 +234
==========================================
+ Hits 159813 160167 +354
+ Misses 28292 28172 -120 |
|
almost lgtm. |
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (4)
src/pipeline/src/manager/util.rs (3)
26-35: Handle potential errors into_pipeline_versionThe function correctly handles valid and invalid versions. However, consider logging the error for debugging purposes.
- .map_err(|_| InvalidPipelineVersionSnafu { version }.build())?; + .map_err(|e| { + eprintln!("Error parsing timestamp: {:?}", e); + InvalidPipelineVersionSnafu { version }.build() + })?;
37-49: Optimize the conditional logic inbuild_plan_filterThe current implementation is clear, but you can simplify the conditional logic using
if letwith anelse.- if let Some(v) = version { - and( - schema_and_name_filter, - col(PIPELINE_TABLE_CREATED_AT_COLUMN_NAME).eq(lit(v.0.to_iso8601_string())), - ) - } else { - schema_and_name_filter - } + version.map_or(schema_and_name_filter, |v| { + and( + schema_and_name_filter, + col(PIPELINE_TABLE_CREATED_AT_COLUMN_NAME).eq(lit(v.0.to_iso8601_string())), + ) + })
52-61: Consider usingformat!forgenerate_pipeline_cache_keyThe
format!macro is used correctly, but consider adding comments to explain the logic for clarity.// Generate cache key with version or 'latest' if version is None match version { Some(version) => format!("{}/{}/{}", schema, name, i64::from(version)), None => format!("{}/{}/latest", schema, name), }src/pipeline/src/manager/table.rs (1)
318-391: Add detailed logging for pipeline deletionThe
delete_pipelinemethod performs several critical steps. Add detailed logging at each step to aid in debugging and maintenance.info!("Checking if pipeline exists in catalog: {}, version: {:?}", name, version); let pipeline = self.find_pipeline(schema, name, version).await?; if pipeline.is_none() { info!("Pipeline not found: {}, version: {:?}", name, version); return Ok(()); } info!("Pipeline found: {}, version: {:?}", name, version);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (7)
- src/pipeline/src/lib.rs (1 hunks)
- src/pipeline/src/manager.rs (1 hunks)
- src/pipeline/src/manager/pipeline_operator.rs (6 hunks)
- src/pipeline/src/manager/table.rs (8 hunks)
- src/pipeline/src/manager/util.rs (1 hunks)
- src/pipeline/src/metrics.rs (1 hunks)
- src/servers/src/http/event.rs (8 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/pipeline/src/lib.rs
- src/pipeline/src/manager.rs
- src/servers/src/http/event.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/frontend/src/instance/log_handler.rs (2 hunks)
- src/pipeline/src/manager/pipeline_operator.rs (6 hunks)
- src/pipeline/src/manager/table.rs (9 hunks)
- src/servers/src/http/event.rs (7 hunks)
- src/servers/src/query_handler.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/frontend/src/instance/log_handler.rs
- src/pipeline/src/manager/pipeline_operator.rs
- src/servers/src/http/event.rs
- src/servers/src/query_handler.rs
Additional comments not posted (8)
src/pipeline/src/manager/table.rs (8)
28-29: Approved: Necessary imports added.The imports for
colandTableReferenceare necessary for the logical plan and table reference in the newdelete_pipelinemethod.
47-53: Approved: Updated error handling and utility imports.The imports for error handling and utility functions are necessary for the new method implementations.
55-65: Approved: Addition of pipeline table column constants and cache settings.The addition of constants for pipeline table columns and cache settings improves code readability and maintainability.
Line range hint
68-82: Approved: Addition of cache toPipelineTablestruct.The addition of a cache to the
PipelineTablestruct improves performance by storing compiled pipelines.
251-255: Approved: Addition of logging for successful pipeline insertions.Logging successful pipeline insertions helps in monitoring and debugging.
268-282: Approved: Caching of compiled pipelines inget_pipelinemethod.Caching compiled pipelines improves performance by avoiding redundant compilations.
305-311: Approved: Caching compiled pipelines with and without version ininsert_and_compilemethod.Caching pipelines with and without version ensures that the latest and specific versions are quickly accessible.
317-393: Approved: Addition ofdelete_pipelinemethod with necessary checks and cache removal.The
delete_pipelinemethod includes necessary checks for pipeline existence, logical plan creation, execution, and cache removal. This addition enhances the pipeline management functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/servers/src/http/greptime_manage_resp.rs (1)
74-96: Fix the typo in the comment.The
IntoResponseimplementation is correct. However, there is a typo in the comment.- // We delibrately don't add this format into [`crate::http::ResponseFormat`] + // We deliberately don't add this format into [`crate::http::ResponseFormat`]Tools
GitHub Check: Check typos and docs
[warning] 80-80:
"delibrately" should be "deliberately".
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/pipeline/tests/gsub.rs (1 hunks)
- src/pipeline/tests/pipeline.rs (1 hunks)
- src/servers/src/http.rs (4 hunks)
- src/servers/src/http/event.rs (8 hunks)
- src/servers/src/http/greptime_manage_resp.rs (1 hunks)
- tests-integration/tests/http.rs (3 hunks)
Files skipped from review due to trivial changes (2)
- src/pipeline/tests/gsub.rs
- src/pipeline/tests/pipeline.rs
Files skipped from review as they are similar to previous changes (3)
- src/servers/src/http.rs
- src/servers/src/http/event.rs
- tests-integration/tests/http.rs
Additional context used
GitHub Check: Check typos and docs
src/servers/src/http/greptime_manage_resp.rs
[warning] 80-80:
"delibrately" should be "deliberately".
Additional comments not posted (4)
src/servers/src/http/greptime_manage_resp.rs (4)
1-23: Header comments and imports look good.The standard Apache License header comments and necessary imports are correctly included.
24-58: Struct definition and methods look good.The
GreptimedbManageResponsestruct and its methods are well-defined and appropriately handle serialization and deserialization.
60-72: Enum and struct definitions look good.The
ManageResultenum andPipelineOutputstruct are well-defined. The TODO comment for future refactoring is noted.
99-136: Test module looks good.The test case for the
into_responsemethod is comprehensive and correctly validates the method.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/servers/src/http/greptime_manage_resp.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/servers/src/http/greptime_manage_resp.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (1)
- tests-integration/tests/http.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests-integration/tests/http.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/script/src/python/rspython/builtins/test.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- src/script/src/python/rspython/builtins/test.rs
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This pr mainly
mod.rstoxxx.rsChecklist
Summary by CodeRabbit
New Features
GreptimedbManageResponsefor better manage API responses, including execution times and formatted outputs.ManageResultandPipelineOutputto enhance script and pipeline management responses.Tests
test_pipeline_api.Documentation
convert_scalar_to_py_obj_and_back()andtest_vm().