feat: delete indexer from cluster #1284
Conversation
WalkthroughThe changes update the naming and functionality for deleting metadata associated with nodes. The previously ingestor-specific interfaces and functions are now made generic to cover both ingestors and indexers. The core removal function has been renamed from Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant QS as QueryServer
participant CH as Cluster Handler
participant OS as Object Storage
C->>QS: DELETE /{node_url} (request deletion)
QS->>CH: Forward request to remove_node
CH->>CH: Validate node liveness and determine metadata type
CH->>OS: Call remove_node_metadata for node deletion
OS-->>CH: Return success or error from try_delete_node_meta
CH-->>QS: Return deletion result
QS-->>C: Send final response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/storage/s3.rs (1)
730-749: Update parameter name and error message for consistencyThe function name has been updated to reflect generalized node handling, but there are still inconsistencies in parameter naming and error messaging.
Consider updating the following for better consistency:
async fn try_delete_node_meta( - &self, - ingestor_filename: String, + &self, + node_filename: String, ) -> Result<(), ObjectStorageError> { - let file = RelativePathBuf::from(&ingestor_filename); + let file = RelativePathBuf::from(&node_filename); match self.client.delete(&to_object_store_path(&file)).await { Ok(_) => Ok(()), Err(err) => { // if the object is not found, it is not an error // the given url path was incorrect if matches!(err, object_store::Error::NotFound { .. }) { error!("Node does not exist"); Err(err.into()) } else { - error!("Error deleting ingestor meta file: {:?}", err); + error!("Error deleting node meta file: {:?}", err); Err(err.into()) } } } }src/handlers/http/modal/query_server.rs (1)
336-344: Update route path and comment to match node terminologyThe function call has been updated to use
remove_nodeinstead ofremove_ingestor, but the route path and comment still use "ingestor" terminology.Consider updating the route path and comment to maintain terminology consistency:
- // DELETE "/cluster/{ingestor_domain:port}" ==> Delete an ingestor from the cluster + // DELETE "/cluster/{node_url:port}" ==> Delete a node from the cluster .service( - web::scope("/{ingestor}").service( + web::scope("/{node_url}").service( web::resource("").route( web::delete() .to(cluster::remove_node) .authorize(Action::Deleteingestor), ), ), )Additionally, consider updating the authorization action name from
Action::Deleteingestorto a more genericAction::DeleteNodein a future PR.src/storage/localfs.rs (1)
328-334: Rename parameter for consistency with function nameThe function name has been updated to reflect the generalized node handling, but the parameter name still refers specifically to "ingestor".
Consider updating the parameter name for consistency:
async fn try_delete_node_meta( &self, - ingestor_filename: String, + node_filename: String, ) -> Result<(), ObjectStorageError> { - let path = self.root.join(ingestor_filename); + let path = self.root.join(node_filename); Ok(fs::remove_file(path).await?) }src/storage/azure_blob.rs (1)
647-666: Update parameter name and error message for consistencyThe function name has been updated to reflect generalized node handling, but there are still inconsistencies in parameter naming and error messaging.
Consider updating the following for better consistency:
async fn try_delete_node_meta( &self, - ingestor_filename: String, + node_filename: String, ) -> Result<(), ObjectStorageError> { - let file = RelativePathBuf::from(&ingestor_filename); + let file = RelativePathBuf::from(&node_filename); match self.client.delete(&to_object_store_path(&file)).await { Ok(_) => Ok(()), Err(err) => { // if the object is not found, it is not an error // the given url path was incorrect if matches!(err, object_store::Error::NotFound { .. }) { error!("Node does not exist"); Err(err.into()) } else { - error!("Error deleting ingestor meta file: {:?}", err); + error!("Error deleting node meta file: {:?}", err); Err(err.into()) } } } }src/storage/object_storage.rs (1)
204-207: Renamed method for consistent node terminologyThe method has been renamed from
try_delete_ingestor_metatotry_delete_node_metawhile maintaining the same signature. This change aligns with the shift to more generic node handling in the codebase, allowing for consistent treatment of both ingestor and indexer nodes.Note that the parameter name
ingestor_filenameremains unchanged, which could be confusing since the method now handles both ingestor and indexer metadata.Consider renaming the parameter from
ingestor_filenametonode_filenamefor consistency with the method name:async fn try_delete_node_meta( &self, - ingestor_filename: String, + node_filename: String, ) -> Result<(), ObjectStorageError>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/handlers/http/cluster/mod.rs(5 hunks)src/handlers/http/modal/mod.rs(3 hunks)src/handlers/http/modal/query_server.rs(1 hunks)src/storage/azure_blob.rs(1 hunks)src/storage/localfs.rs(1 hunks)src/storage/object_storage.rs(1 hunks)src/storage/s3.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/handlers/http/modal/query_server.rs (1)
src/handlers/http/cluster/mod.rs (1)
remove_node(710-736)
src/storage/s3.rs (3)
src/storage/azure_blob.rs (1)
try_delete_node_meta(647-666)src/storage/object_storage.rs (1)
try_delete_node_meta(204-207)src/storage/localfs.rs (1)
try_delete_node_meta(328-334)
src/storage/object_storage.rs (3)
src/storage/localfs.rs (1)
try_delete_node_meta(328-334)src/storage/azure_blob.rs (1)
try_delete_node_meta(647-666)src/storage/s3.rs (1)
try_delete_node_meta(730-749)
src/storage/azure_blob.rs (3)
src/storage/s3.rs (1)
try_delete_node_meta(730-749)src/storage/object_storage.rs (1)
try_delete_node_meta(204-207)src/storage/localfs.rs (1)
try_delete_node_meta(328-334)
src/handlers/http/cluster/mod.rs (2)
src/handlers/http/cluster/utils.rs (6)
to_url_string(197-204)check_liveness(175-195)new(35-47)new(62-80)new(95-113)new(125-132)src/storage/mod.rs (4)
default(205-225)from(157-163)new(182-184)new(195-201)
src/handlers/http/modal/mod.rs (1)
src/option.rs (1)
file_path(85-97)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (7)
src/handlers/http/modal/mod.rs (3)
556-556: Added new method to Metadata trait for consistent file path accessThe addition of the
file_pathmethod to theMetadatatrait provides a standardized way to retrieve the file path for different node types. This is a good abstraction that will make it easier to handle different node types uniformly when working with node metadata.
570-572: Potential method name confusion but works as expectedThe implementation for
IngestorMetadatacalls the struct's own method with the same name, which could look like recursive calls at first glance. However, Rust can differentiate between trait methods and struct methods with the same name, so this implementation works as expected.
586-588: Consistent implementation across node typesThe implementation for
IndexerMetadatafollows the same pattern as that forIngestorMetadata, ensuring consistency between the two node types. This uniformity is good for code maintainability.src/handlers/http/cluster/mod.rs (4)
647-649: Added check for empty node listsThis check is a good defensive programming practice that prevents unnecessary processing when the input list is empty. It improves efficiency by avoiding the overhead of setting up parallel processing for an empty collection.
710-736: Refactored function to handle multiple node typesThe function has been renamed from
remove_ingestortoremove_nodeand updated to handle both ingestor and indexer node types. This change enables the deletion of any node type from the cluster, enhancing the functionality as described in the PR objective.The error message has also been updated to be more generic ("node" instead of "ingestor").
739-775: Added a reusable helper function for node metadata removalThe new
remove_node_metadatafunction is a well-designed generic helper that works with any node type implementing the required traits. It:
- Gets the node type from the default instance
- Fetches relevant metadata objects from storage
- Filters for the specific domain name
- Attempts to delete the metadata file
- Properly handles errors and provides meaningful feedback
Using generics here allows for code reuse and ensures future node types can be handled without duplicating logic.
764-772: Successfully using the new trait method for consistent file path handlingThis code demonstrates the benefit of adding the
file_path()method to theMetadatatrait. By callingnode_metadatas[0].file_path().to_string(), the code works with any node type without needing to know its specific implementation details.Also, the function correctly uses the renamed
try_delete_node_metamethod from theObjectStoragetrait, showing good consistency across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/coverage.yaml (1)
39-40: Environment Variable Update for Clippy Check
The updated environment variables (RUSTFLAGSandCMAKE_ARGS) appropriately replace the oldCMAKE_FLAGS. Please verify that:
- The syntax for
RUSTFLAGS(including the embedded quotes) is correctly interpreted by downstream Rust tooling.- Downstream CI steps or scripts that previously depended on
CMAKE_FLAGSare updated accordingly to useCMAKE_ARGS.Consider adding a brief comment in the YAML file referencing why these variables were updated to aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/coverage.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
DELETE /cluster/{node_url} to handle deletion of all nodes
including ingestor and indexer
nikhilsinhaparseable
force-pushed
the
delete-indexer
branch
from
5f2aff7 to
caa6eb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/storage/localfs.rs (1)
328-331: Function renamed to align with node generalization.The function has been renamed from
try_delete_ingestor_metatotry_delete_node_metaas part of the broader refactoring to generalize "ingestor" to "node" across the codebase. This change allows the function to be used for both ingestor and indexer nodes, supporting the new feature to delete all nodes within a cluster.The implementation now correctly uses
fs::remove_fileinstead offs::remove_dir_all(according to the AI summary), which suggests a change in how node metadata is stored - from a directory to a file structure. This is a significant behavior change.Consider adding a function-level documentation comment to clarify the purpose of this function and its role in node deletion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/storage/localfs.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
DELETE /cluster/{node_url} to handle deletion of all nodes
including ingestor and indexer
Summary by CodeRabbit
New Features
Refactor