feat: add querier json #1288
feat: add querier json #1288
Conversation
WalkthroughThis pull request introduces extensive changes to incorporate a new operational mode, Prism, across several modules. It adds new metrics in the analytics layer, refactors metadata retrieval by replacing specific functions (e.g., Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 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 (10)
🪧 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/handlers/http/cluster/mod.rs (4)
124-127: Repetitive code for retrieving ingestor info.Multiple calls to
get_node_info("ingestor")appear in this file. Consider encapsulating this repeated pattern in a helper if more logic is ever needed for ingestor retrieval.
174-177: Same retrieval pattern repeated.Again, you might consider a small helper for repeated node retrieval calls to promote DRY principles.
279-282: Same code call for ingestor retrieval again.Consolidating repeated calls can reduce future maintenance overhead.
684-700: Potential silent parsing errors inget_node_info.Using
.filter_map(|x| serde_json::from_slice::<T>(x).ok())discards parse errors silently. It might be beneficial to log parse failures for easier debugging, rather than ignoring them outright..iter() - .filter_map(|x| serde_json::from_slice::<T>(x).ok()) + .filter_map(|x| { + match serde_json::from_slice::<T>(x) { + Ok(parsed) => Some(parsed), + Err(e) => { + tracing::warn!("Failed to parse node info: {:?}", e); + None + } + } + })src/handlers/http/modal/mod.rs (1)
245-246:#[allow(clippy::too_many_arguments)]on constructor.Acceptable for struct initialization; consider grouping related fields into small config objects if it grows further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/analytics.rs(2 hunks)src/catalog/mod.rs(2 hunks)src/cli.rs(2 hunks)src/handlers/airplane.rs(2 hunks)src/handlers/http/cluster/mod.rs(13 hunks)src/handlers/http/cluster/utils.rs(4 hunks)src/handlers/http/mod.rs(2 hunks)src/handlers/http/modal/mod.rs(11 hunks)src/handlers/http/modal/query/querier_logstream.rs(2 hunks)src/handlers/http/modal/query_server.rs(2 hunks)src/parseable/mod.rs(8 hunks)src/rbac/role.rs(1 hunks)src/storage/object_storage.rs(3 hunks)src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
src/handlers/http/modal/query/querier_logstream.rs (1)
src/handlers/http/cluster/mod.rs (1)
get_node_info(684-700)
src/analytics.rs (1)
src/handlers/http/cluster/mod.rs (1)
get_node_info(684-700)
src/utils/mod.rs (1)
src/handlers/http/modal/mod.rs (1)
get_node_id(369-371)
src/parseable/mod.rs (2)
src/handlers/http/modal/mod.rs (1)
load(272-367)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
src/handlers/airplane.rs (1)
src/handlers/http/cluster/mod.rs (1)
get_node_info(684-700)
src/catalog/mod.rs (2)
src/handlers/http/mod.rs (1)
base_path_without_preceding_slash(76-78)src/handlers/http/cluster/mod.rs (1)
get_node_info(684-700)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(37-41)src/parseable/mod.rs (2)
new(145-184)storage(257-259)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- 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
🔇 Additional comments (60)
src/rbac/role.rs (1)
343-343: LGTM - Added proper newline at end of file.The addition of a newline character at the end of the file follows good coding practices.
src/handlers/http/modal/query_server.rs (2)
28-28: LGTM - Added necessary import.Added the
Modeenum import to support the querier metadata storage functionality.
133-135: LGTM - Added querier metadata storage.Added the functionality to store the querier metadata to storage during initialization, which completes the implementation for querier node type in the distributed setup. This aligns with the PR's objective to standardize node metadata retrieval and handling.
src/storage/object_storage.rs (3)
904-904: LGTM - Standardizing node identifier retrieval.Changed from specific
get_ingestor_id()to the more genericget_node_id()method, which aligns with the standardized node metadata approach.
923-923: LGTM - Standardizing node identifier retrieval.Changed from specific
get_ingestor_id()to the more genericget_node_id()method, maintaining consistency with the standardized approach.
973-973: LGTM - Standardizing node identifier retrieval.Changed from specific
get_ingestor_id()to the more genericget_node_id()method, completing the standardization across all file path generation functions.src/cli.rs (2)
331-337: LGTM - Added querier endpoint configuration.Added a new configuration field for the querier endpoint, similar to the existing ingestor and indexer endpoint fields. This provides consistent configuration options across all node types.
481-494: LGTM - Added querier URL handling.Added the handling for
Mode::Queryin theget_urlmethod, following the same pattern as the existing ingestor and indexer endpoint handling. This ensures consistent URL generation for all node types.src/utils/mod.rs (1)
37-41: Good function renaming for improved generality.The function has been renamed from
get_ingestor_id()toget_node_id(), which aligns with the PR's goal of standardizing node metadata retrieval across the codebase. This makes the function more generic and reusable for different node types.src/handlers/http/modal/query/querier_logstream.rs (2)
44-44: Good import addition for the new metadata structure.Adding the
NodeMetadataimport supports the standardized approach to node metadata handling that's being implemented in this PR.
85-89: Appropriate update to use the new generic metadata retrieval function.Replacing
cluster::get_ingestor_info()withcluster::get_node_info("ingestor")and explicitly typing the result asVec<NodeMetadata>improves type safety and aligns with the PR's goal of standardizing node metadata retrieval.src/handlers/http/mod.rs (3)
22-22: Good import update for the new metadata retrieval function.Updating the import from
get_ingestor_infotoget_node_infoaligns with the PR's standardization goals.
25-25: Appropriate import for the new metadata structure.Adding the
NodeMetadataimport is necessary for the type annotation in the updated function call.
113-113: Enhanced type safety with explicit type annotation.Explicitly typing
imaasVec<NodeMetadata>and updating the function call toget_node_info("ingestor")improves code clarity and type safety.src/catalog/mod.rs (2)
31-34: Well-structured import organization.The updated imports properly include
NodeMetadatafrom the modal module, maintaining clean code organization while supporting the PR's standardization goals.
412-418: Consistent implementation of the standardized metadata retrieval approach.The code correctly replaces
handlers::http::cluster::get_ingestor_info()withhandlers::http::cluster::get_node_info("ingestor")and adds explicit type annotation, maintaining consistent error handling while aligning with the PR's standardization goals.src/analytics.rs (2)
39-39: Updated import for new NodeMetadata type.The import now uses the
NodeMetadatatype from themodalmodule, which is part of the standardization of node metadata structures across the codebase.
232-232: Migrated from specific to generic node info retrieval.The code now uses the more generic
get_node_info("ingestor")function instead of the previousget_ingestor_info()function. This change is consistent with the broader refactoring effort to standardize node metadata retrieval using a unified approach.src/handlers/airplane.rs (2)
36-37: Updated imports to use the new generic node info retrieval mechanism.Replaced the import of the specific
get_ingestor_infofunction with the more genericget_node_infofunction, and added an import for theNodeMetadatatype.
183-183: Migrated to generic node info retrieval with explicit typing.The code now uses
get_node_info("ingestor")instead ofget_ingestor_info(), with explicit type declaration foringester_metadatasasVec<NodeMetadata>. This change standardizes the metadata retrieval mechanism and improves type safety.src/handlers/http/cluster/utils.rs (4)
19-22: Restructured imports and added NodeType.Imports have been reorganized into a more structured format using nested braces, and the
NodeTypeimport has been added from themodalmodule. This supports the type safety improvements in the struct below.
61-61: Enhanced type safety by using NodeType enum.Changed the
node_typefield type from a genericStringto the more specificNodeTypeenum, which provides better type safety and clearer semantics.
72-73: Updated constructor parameter to match the new type.Changed the type of the
node_typeparameter from&strto&NodeTypeto match the field type change, which ensures type-safe construction ofClusterInfoobjects.
81-82: Updated field assignment to use clone for the NodeType.Now using the
clone()method on theNodeTypereference since it's a more complex type rather than a simple string. This correctly creates a new instance of the enum for the struct field.src/parseable/mod.rs (5)
50-53: Enhanced imports for expanded node metadata support.Updated the import list to include
QuerierMetadataalongside the existing metadata types, which supports the addition of querier-specific metadata handling in theParseablestruct.
134-134: Added querier metadata field to support query node role.Added a new field
querier_metadatato theParseablestruct to hold metadata specific to a querier node. This complements the existing fields for ingestors and indexers, providing consistent metadata handling across all node types.
151-172: Standardized node metadata loading across all node types.Updated the conditional initialization of metadata for all node types (ingestor, indexer, querier) to use the generic
loadmethod with specificNodeTypeenum values. This creates a consistent pattern for loading metadata across different node roles.
289-324: Simplified metadata storage with a unified approach.Refactored the
store_metadatamethod to use a single match statement that handles all node types consistently, reducing code duplication and improving maintainability. The method now:
- Retrieves the appropriate metadata reference based on the node type
- Updates domain and port if needed
- Stores the metadata in the object store
This approach is more modular and easier to extend for future node types.
209-209: Standardized node ID retrieval.Changed calls from
get_ingestor_id()toget_node_id()to use a more generic identifier for nodes, which aligns with the overall refactoring to standardize node metadata handling. This makes the code more consistent and future-proof.Also applies to: 385-385
src/handlers/http/cluster/mod.rs (17)
57-57: Imports look consistent.The expanded import list for node metadata types aligns with the new refactored approach.
76-79: Reusingget_node_info("ingestor")is good.Ensure there are no lingering calls to the old
get_ingestor_infofunction elsewhere.
221-224: Ingestor info retrieval is consistent.The pattern is correct. Ensure error handling remains consistent across all calls.
327-330: Repeated pattern for syncing node info.All looks correct. Just remain mindful of duplication if future logic expands.
544-550: Parallel metadata fetching for all node types.Using
future::join3is a concise approach. Errors for each future propagate properly.
551-557: Handling querier metadata result.The error path logs the failure clearly. Good job ensuring logs capture the issue.
560-565: Handling ingestor metadata with consistent error mapping.Approach is consistent; no improvements needed here.
568-573: Handling indexer metadata result.Error is converted to
StreamError::Anyhow. This remains consistent with the rest of the flow.
576-580: Fetching node info concurrently.Good pattern for parallel concurrency.
585-587: Merging results into a single vector.The extension order won't affect correctness. The approach is fine.
720-723: Removal of querier metadata.This new step ensures all node types are removed consistently.
724-724: Unified removal logic for multiple node types.Combining results with
if removed_ingestor || removed_indexer || removed_querieris clear.
835-837: Early return if no nodes exist.Short-circuiting on an empty list conserves resources.
860-865: Fetching querier, ingestor, and indexer metadata in parallel.Code is succinct and concurrency-friendly.
867-871: Querier metadata error handling.Logs are explicit, ensuring quick debugging if this fails.
883-887: Gathering node metrics concurrently.Parallel fetching from all node types is a good pattern.
893-898: Consolidated error handling for querier metrics.Cleanly merges success cases, returns on error. This is consistent with the rest of the method.
src/handlers/http/modal/mod.rs (14)
19-19: New import forfmtandArcusage.This is consistent with the expanded usage for
NodeTypeandNodeMetadata.
45-45: Import ofget_node_idis appropriate.Ensures node IDs can be generated for new metadata objects.
61-61: Updating default version tov4.This aligns with the new migration logic.
202-211: Introduction ofNodeTypeenum.Declaring an explicit
NodeTypeimproves clarity and extends type safety for node metadata.
213-221:as_strmethod forNodeType.String representations are consistent with the serde rename attributes.
223-231:fmt::Displayimplementation forNodeType.The output matches
as_strusage. This is good for debugging logs and clarity.
234-243:NodeMetadatastruct unifies metadata.Replacing
(Ingestor|Indexer|Querier)Metadatawith a singleNodeMetadataplus anode_typeenum is a cleaner design.
500-519: Helper for creating ingestor metadata.This matches the newly introduced
NodeMetadata::newsignature, ensuring a simpler instantiation pattern for ingestors.
521-526: Loading ingestor metadata with dedicated helper.Keeps the code DRY for that node type.
528-547: Helper for creating indexer metadata.Mirrors the ingestor approach. All good.
549-554: Loading indexer metadata.Consistent with the other node types, ensuring standardized usage.
556-575: Helper for creating querier metadata.Parallel design to ingestor/indexer creation. Looks good.
577-581: Loading querier metadata.Completes the set of loading helpers for each node type.
588-606: Additional test coverage references.Tests confirm correct enum usage, flight port preservation, and migration from older versions. Good approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/handlers/http/modal/mod.rs (1)
343-344: Use node_type assignment instead of clone_from.Using direct assignment would be clearer and potentially more efficient than clone_from for the enum type.
- meta.node_type.clone_from(&node_type); + meta.node_type = node_type;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/modal/mod.rs(11 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(37-41)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (14)
src/handlers/http/modal/mod.rs (14)
61-61: Versioning update to support new unified node structure.The version has been updated from "v3" to "v4" to accommodate the new unified NodeMetadata structure and support for querier nodes.
202-211: Well-designed enum for node types with proper serialization support.The NodeType enum provides type safety and clear representation of different node roles (Ingestor, Indexer, Querier). Good use of serde attributes for serialization/deserialization and setting Ingestor as the default.
213-221: Clean implementation of as_str method.This implementation provides a consistent way to get string representations of node types, which will be useful for file paths and logging.
223-231: Proper Display trait implementation.The Display trait implementation allows for easy formatting of NodeType in logs and user-facing messages.
233-243: Good consolidation of metadata into a unified structure.The NodeMetadata struct now uses a generic node_id and node_type field instead of specific fields for each node type, which simplifies the codebase and makes it more maintainable.
245-269: Updated constructor to support the unified node structure.The new method has been properly updated to accommodate the node_id and node_type fields.
272-290: Flexible node loading with dynamic mode determination.The load method now accepts a node_type parameter and dynamically determines the appropriate mode, making it more versatile.
297-307: Improved file detection logic.The code now uses node_type.as_str() to identify relevant metadata files, which is more flexible than hardcoded type names.
384-416: Robust migration logic for metadata versioning.The migration logic properly handles the transition from v3 to v4 by migrating ingestor_id and indexer_id fields to node_id and adding the appropriate node_type.
429-451: Updated migration method with node type handling.The migrate method now properly preserves the node type during migration.
460-461: Improved file naming convention.The file_name now includes the node type, which makes it easier to identify the file's purpose and supports multiple node types in the same directory.
494-497: Good use of type aliases for backward compatibility.The type aliases for IngestorMetadata, IndexerMetadata, and QuerierMetadata ensure that existing code will continue to work with the new implementation.
499-581: Helper functions simplify the transition to the new API.These helper functions maintain compatibility with existing code that expects specific creation and loading functions for each node type.
However, there's some duplication across these functions. Consider a future refactoring to reduce duplication while maintaining the same API.
605-605: Updated test assertions with node_type field.The test has been properly updated to include the node_type field in the serialized JSON, ensuring compatibility with the new structure.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/handlers/http/modal/mod.rs (1)
303-330: Improve error handling in load_from_staging.Good improvement in error handling for metadata extraction compared to the previous version. However, there are still several
expect()calls that could lead to panics in production.Consider using
map_errand proper error propagation:- let entries = staging_path - .read_dir() - .expect("Couldn't read from staging directory"); + let entries = match staging_path.read_dir() { + Ok(entries) => entries, + Err(e) => { + error!("Couldn't read from staging directory: {}", e); + return None; + } + }; - let path = entry.expect("Should be a directory entry").path(); + let path = match entry { + Ok(entry) => entry.path(), + Err(e) => { + error!("Error reading directory entry: {}", e); + continue; + } + };
🧹 Nitpick comments (1)
src/handlers/http/modal/mod.rs (1)
412-447: Refactor the migration logic to reduce duplication.The migration logic for different node types contains duplicated code patterns that could be extracted into a helper function.
Consider refactoring to reduce duplication:
fn from_bytes(bytes: &[u8], flight_port: u16) -> anyhow::Result<Self> { let mut json: Map<String, Value> = serde_json::from_slice(bytes)?; // Check version let version = json.get("version").and_then(|version| version.as_str()); if version == Some("v3") { - if json.contains_key("ingestor_id") { - // Migration: get ingestor_id value, remove it, and add as node_id - if let Some(id) = json.remove("ingestor_id") { - json.insert("node_id".to_string(), id); - json.insert( - "version".to_string(), - Value::String(DEFAULT_VERSION.to_string()), - ); - } - json.insert( - "node_type".to_string(), - Value::String("ingestor".to_string()), - ); - } else if json.contains_key("indexer_id") { - // Migration: get indexer_id value, remove it, and add as node_id - if let Some(id) = json.remove("indexer_id") { - json.insert("node_id".to_string(), id); - json.insert( - "version".to_string(), - Value::String(DEFAULT_VERSION.to_string()), - ); - } - json.insert( - "node_type".to_string(), - Value::String("indexer".to_string()), - ); - } + // Helper function to migrate legacy ID fields + fn migrate_legacy_id(json: &mut Map<String, Value>, legacy_id_key: &str, node_type_str: &str) -> bool { + if json.contains_key(legacy_id_key) { + if let Some(id) = json.remove(legacy_id_key) { + json.insert("node_id".to_string(), id); + json.insert( + "version".to_string(), + Value::String(DEFAULT_VERSION.to_string()), + ); + } + json.insert( + "node_type".to_string(), + Value::String(node_type_str.to_string()), + ); + true + } else { + false + } + } + + if !migrate_legacy_id(&mut json, "ingestor_id", "ingestor") { + migrate_legacy_id(&mut json, "indexer_id", "indexer"); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/modal/mod.rs(10 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/handlers/http/modal/mod.rs (1)
src/utils/mod.rs (1)
get_node_id(37-41)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (6)
src/handlers/http/modal/mod.rs (6)
202-211: Good addition of a NodeType enum for better type safety.The introduction of the NodeType enum improves type safety and code clarity by replacing string-based type checking with enum variants. The serde rename attributes ensure proper serialization/deserialization.
213-228: Well-structured helper methods for NodeType.The
as_str()andto_mode()methods provide clean and consistent conversions between node types and their string representations or operational modes, eliminating error-prone string manipulations.
242-251: Good refactoring of metadata structure.Consolidating different node type metadata into a single
NodeMetadatastructure with a discriminator field is a cleaner approach than maintaining separate structures for each node type.
525-528: Good use of type aliases for backward compatibility.The type aliases provide a clean way to maintain backward compatibility while refactoring the codebase to use the new unified
NodeMetadatastructure.
530-612: Well-designed helper functions for creating and loading node metadata.These helper functions provide a clean API for creating and loading different types of node metadata while abstracting away the implementation details of the unified
NodeMetadatastructure.
61-61:❓ Verification inconclusive
Verify version bump implications.
The
DEFAULT_VERSIONconstant has been updated from "v3" to "v4". Ensure all code paths handling version-specific behaviors have been updated accordingly, including migration code and any external consumers of this API.
🏁 Script executed:
#!/bin/bash # Check for other references to version strings in the codebase rg -A 3 -B 3 '"v3"|"v4"'Length of output: 10828
Action: Confirm Version Bump Impact & Verify External Consumer Alignment
- The
DEFAULT_VERSIONconstant insrc/handlers/http/modal/mod.rsis now"v4", and the migration functions (e.g.,v3_v4insrc/migration/metadata_migration.rsand corresponding migration logic insrc/migration/mod.rs) correctly transform legacy"v3"data to"v4". Tests in the same module also verify that JSON inputs with"v3"migrate properly.- However, note that other parts of the codebase still reference
"v3"(for example, theCURRENT_DASHBOARD_VERSIONinsrc/users/dashboards.rs). Please verify that these references are either intended for backward compatibility or also need to be updated to align with the new default version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/handlers/http/modal/mod.rs (3)
231-239: Consider reducing duplication by using as_str() in Display implementation.The
Displayimplementation forNodeTypeduplicates the string literal mapping logic that's already present in theas_str()method. This creates a maintenance risk if the string representations need to change in the future.impl fmt::Display for NodeType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - NodeType::Ingestor => write!(f, "ingestor"), - NodeType::Indexer => write!(f, "indexer"), - NodeType::Querier => write!(f, "querier"), - } + write!(f, "{}", self.as_str()) } }
429-457: Add support for Querier in migration logic.The migration code handles legacy
ingestor_idandindexer_idfields, but doesn't account for potentialquerier_idfields. While this might be a new addition (explaining the absence), adding this case would make the migration logic more complete and future-proof.if !migrate_legacy_id(&mut json, "ingestor_id", "ingestor") { - migrate_legacy_id(&mut json, "indexer_id", "indexer"); + if !migrate_legacy_id(&mut json, "indexer_id", "indexer") { + migrate_legacy_id(&mut json, "querier_id", "querier"); + } }
622-684: Consider expanding test coverage for new node types.The test suite currently only verifies
IngestorMetadatafunctionality. Since you've addedIndexerandQueriernode types, it would be beneficial to add tests specifically for these types to ensure they behave as expected.Consider adding tests that verify:
- Serialization/deserialization of
IndexerMetadataandQuerierMetadata- Migration logic for each node type
- File path generation for each node type
For example:
#[rstest] fn test_node_type_serialization() { assert_eq!(serde_json::to_string(&NodeType::Indexer).unwrap(), "\"indexer\""); assert_eq!(serde_json::to_string(&NodeType::Querier).unwrap(), "\"querier\""); } #[rstest] fn test_file_path_by_node_type() { let im = create_ingestor_metadata("8000".to_string(), "url".to_string(), "bucket".to_string(), "user", "pass", "id".to_string(), "8002".to_string()); let qm = create_querier_metadata("8000".to_string(), "url".to_string(), "bucket".to_string(), "user", "pass", "id".to_string(), "8002".to_string()); assert!(im.file_path().as_str().contains("ingestor")); assert!(qm.file_path().as_str().contains("querier")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/modal/mod.rs(10 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(37-41)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- 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 Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (3)
src/handlers/http/modal/mod.rs (3)
280-301: Excellent handling of critical operations using expect().The code properly maintains the current approach of using
expect()for critical operations like writing metadata to disk, which is the appropriate design as discussed in previous reviews. Letting exceptions propagate for fundamental system issues is the correct approach here.
202-228: Well-designed NodeType enum with clean mappings.The
NodeTypeenum is well-structured with appropriate traits derived and clean mapping methods to convert between different representations. Theto_mode()method provides a clear mapping to the underlying Mode enum.
534-621: Good backward compatibility with type aliases and helper functions.The addition of type aliases and helper functions provides a clean way to maintain backward compatibility while moving to a more generic and flexible architecture. This approach minimizes disruption to existing code while enabling the new node type functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/modal/mod.rs (1)
325-332: Error handling for file reading could be improved.When reading metadata files fails, the implementation may discard potentially useful debugging information.
Consider improving error handling when loading metadata from staging:
- let bytes = std::fs::read(&path).expect("File should be present"); - match Self::from_bytes(&bytes, options.flight_port) { - Ok(meta) => return Some(meta), - Err(e) => { - error!("Failed to extract {} metadata: {}", node_type_str, e); - return None; - } - } + match std::fs::read(&path) { + Ok(bytes) => match Self::from_bytes(&bytes, options.flight_port) { + Ok(meta) => return Some(meta), + Err(e) => { + error!("Failed to extract {} metadata from {}: {}", + node_type_str, path.display(), e); + return None; + } + }, + Err(e) => { + error!("Failed to read metadata file {}: {}", path.display(), e); + return None; + } + }
🧹 Nitpick comments (2)
src/handlers/http/modal/mod.rs (2)
339-343: The file validation method might be too permissive.The current implementation only checks if the filename contains the node type string, which could lead to false positives if other files happen to contain the same substring.
Consider using a more specific pattern match:
fn is_valid_metadata_file(path: &Path, node_type_str: &str) -> bool { - path.file_name() - .and_then(|s| s.to_str()) - .is_some_and(|s| s.contains(node_type_str)) + path.file_name() + .and_then(|s| s.to_str()) + .is_some_and(|s| s.starts_with(&format!("{}.", node_type_str))) }
406-408: Avoid unnecessary string cloning.The
get_node_id()method clones the string unnecessarily, which is inefficient especially for frequently called methods.Return a reference instead of cloning:
- pub fn get_node_id(&self) -> String { - self.node_id.clone() + pub fn get_node_id(&self) -> &str { + &self.node_id }Note: This change would require updating callers that expect ownership of the returned string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/catalog/mod.rs(4 hunks)src/handlers/http/middleware.rs(1 hunks)src/handlers/http/modal/mod.rs(10 hunks)src/main.rs(1 hunks)src/option.rs(1 hunks)src/parseable/mod.rs(8 hunks)src/storage/object_storage.rs(2 hunks)src/storage/store_metadata.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/object_storage.rs
- src/catalog/mod.rs
🧰 Additional context used
🧬 Code Definitions (2)
src/parseable/mod.rs (2)
src/handlers/http/modal/mod.rs (1)
load(276-297)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
src/handlers/http/modal/mod.rs (3)
src/utils/mod.rs (1)
get_node_id(37-41)src/parseable/mod.rs (2)
new(145-184)storage(257-259)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (19)
src/handlers/http/modal/mod.rs (8)
202-211: NodeType enum looks well-designed.The introduction of a structured NodeType enum with proper serialization attributes is a good approach to improve type safety and maintenance. The default variant is appropriately set to Ingestor.
213-229: Good implementation of helper methods for NodeType.The
as_str()andto_mode()methods provide clean conversion between NodeType and its string/mode representations, ensuring type safety and consistency throughout the codebase.
231-235: Clean Display implementation for NodeType.The implementation uses the existing
as_str()method, which promotes code reuse and consistency.
237-247: Good struct design for the consolidated NodeMetadata.The struct now handles all node types with a shared structure, which simplifies the codebase and reduces duplication. The new
node_typefield provides proper type identification.
276-297: Consider handling staging directory write failures more gracefully.Even though the previous feedback discussion established that using
expect()is intentional for critical failures, it might be worth considering a fallback mechanism for temporary I/O issues.Are there any scenarios where a temporary I/O issue could cause this to fail in production? If so, would adding retry logic be beneficial?
422-454: Good migration logic for version compatibility.The implementation properly handles the migration from v3 to v4 metadata format, correctly transforming legacy IDs to the new field names and structure. This ensures backward compatibility.
530-534: Good use of type aliases for backward compatibility.The type aliases for IngestorMetadata, IndexerMetadata, and QuerierMetadata make the transition to the new NodeMetadata structure smoother by maintaining API compatibility.
536-617: Helper functions provide a nice abstraction.These functions encapsulate the creation and loading of specific node types, making the API more user-friendly while hiding the implementation details.
src/option.rs (1)
26-26: Good addition of the Prism variant to Mode enum.Adding the Prism variant is a clean way to extend the system's functionality. The placement before the default All variant maintains the logical structure of the enum.
src/main.rs (1)
46-49: Consistent handling of enterprise features.The implementation for Mode::Prism follows the same pattern as Mode::Index, providing a consistent user experience for enterprise features. Good user-friendly message with a clear call to action.
src/storage/store_metadata.rs (1)
162-162: Efficient code reuse with combined match arm.Using a combined match arm for
Mode::IndexandMode::Prismis an efficient way to handle both modes that require the same logic, avoiding code duplication.src/handlers/http/middleware.rs (1)
361-361: Add support for Mode::Prism in route handlingThe modification updates the match arm to include
Mode::PrismalongsideMode::Index, allowing both modes to follow the same execution path in theModeFilterMiddleware. This change aligns with the broader refactoring effort to unify the handling of different node types.src/parseable/mod.rs (7)
51-53: Include QuerierMetadata in importsThe import statement has been updated to include
QuerierMetadata, which is necessary for the new querier metadata support being added to the system.
134-135: Add querier_metadata field to Parseable structA new field
querier_metadatais added to store metadata specific to querier nodes. This follows the same pattern as the existingingestor_metadataandindexer_metadatafields, maintaining design consistency.
152-155: Update metadata loading with NodeType enumThe metadata loading has been refactored to use the
NodeTypeenum, creating a more type-safe and consistent approach across all node types. This change ensures that each mode loads the appropriate metadata with its corresponding node type identifier.Also applies to: 159-165, 166-172
180-180: Include querier_metadata in struct initializationThe new querier_metadata field is properly included in the Parseable struct initialization.
209-209: Replace get_ingestor_id() with get_node_id()The code now uses the more generic
get_node_id()method instead of the ingestor-specificget_ingestor_id(). This change supports the unified approach to node handling and improves code maintainability by reducing duplication.Also applies to: 386-386, 691-691
285-285: Add string representation for Mode::PrismThe server mode string representation is updated to include
Mode::Prism, ensuring consistent user-facing identification of the new mode.
291-324: Refactor store_metadata method for better maintainabilityThe
store_metadatamethod has been significantly improved:
- Uses a match statement to select the appropriate metadata based on mode
- Simplifies the control flow with early returns
- Consolidates the common logic for storing metadata
- Provides clear separation between different operations
This refactoring reduces code duplication and makes the method more maintainable while accommodating the new querier metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/analytics.rs (1)
114-135: Consider adding error resilience for the analytics flow.While the current implementation correctly handles errors using the
?operator, the analytics flow could be made more resilient by handling errors for individual node types rather than failing the entire Report generation.Consider a more resilient approach:
- let indexer_infos: Vec<NodeMetadata> = cluster::get_node_info("indexer").await?; + let indexer_infos: Vec<NodeMetadata> = match cluster::get_node_info("indexer").await { + Ok(infos) => infos, + Err(err) => { + error!("Failed to fetch indexer info: {}", err); + Vec::new() + } + }; - let query_infos: Vec<NodeMetadata> = cluster::get_node_info("query").await?; + let query_infos: Vec<NodeMetadata> = match cluster::get_node_info("query").await { + Ok(infos) => infos, + Err(err) => { + error!("Failed to fetch query info: {}", err); + Vec::new() + } + }; - let ingestor_infos: Vec<NodeMetadata> = cluster::get_node_info("ingestor").await?; + let ingestor_infos: Vec<NodeMetadata> = match cluster::get_node_info("ingestor").await { + Ok(infos) => infos, + Err(err) => { + error!("Failed to fetch ingestor info: {}", err); + Vec::new() + } + };This would allow the Report to be generated even if one of the node type information retrievals fails.
Also applies to: 261-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/analytics.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (6)
src/analytics.rs (6)
39-39: New import for NodeMetadata looks good.The addition of
NodeMetadatafrom themodalmodule is appropriate for the unified approach to node information retrieval being implemented.
78-81: Report struct extension is well-structured.Adding fields for tracking both active and inactive indexers and queries is a logical extension to the existing ingestor tracking. This provides more comprehensive analytics about the cluster state.
114-135: Node metadata retrieval implementation is consistent.The implementation for fetching and categorizing indexers and queries follows the same pattern used for ingestors, making good use of the new
get_node_info()function. The liveness check approach is consistent across all node types.
151-154: Field initialization is correctly integrated.The new fields are properly added to the Report struct initialization, which completes the integration of the new metrics.
257-257: Mode check update aligns with new Prism mode support.Adding the Mode::Prism check alongside Mode::Query ensures consistent behavior for the new mode type. This update correctly maintains the conditional logic for when to fetch ingestor metrics.
261-261: Unified approach to node information retrieval.Using
cluster::get_node_info("ingestor")which returnsVec<NodeMetadata>instead of the previous method aligns with the PR objective of unifying node metadata retrieval. The error handling with?operator is appropriate for async functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/handlers/http/query.rs (1)
339-341: Consider updating status code mappings for new error variants.The
status_code()implementation doesn't explicitly handle the new error variants, which means they'll fall through to the catch-all case and returnBAD_REQUEST. While this may be appropriate, consider if any of these new errors (especiallyNoAvailableQuerier) should return a different status code likeSERVICE_UNAVAILABLE(503) to better reflect the nature of the error.fn status_code(&self) -> http::StatusCode { match self { QueryError::Execute(_) | QueryError::JsonParse(_) => StatusCode::INTERNAL_SERVER_ERROR, + QueryError::NoAvailableQuerier => StatusCode::SERVICE_UNAVAILABLE, _ => StatusCode::BAD_REQUEST, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/query.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/handlers/http/query.rs (10)
src/storage/s3.rs (2)
from(850-858)from(862-864)src/rbac/user.rs (1)
from(177-186)src/analytics.rs (1)
serde_json(284-284)src/handlers/http/modal/mod.rs (1)
serde_json(641-641)src/parseable/mod.rs (2)
serde_json(344-344)serde_json(350-350)src/storage/object_storage.rs (6)
serde_json(439-439)serde_json(514-514)serde_json(541-541)serde_json(581-581)serde_json(607-607)serde_json(684-684)src/handlers/http/cluster/mod.rs (3)
serde_json(616-616)serde_json(696-696)serde_json(750-750)src/query/mod.rs (1)
serde_json(480-480)src/query/stream_schema_provider.rs (1)
serde_json(517-517)src/utils/arrow/flight.rs (1)
serde_json(45-45)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (1)
src/handlers/http/query.rs (1)
328-333: Good addition of specific error types to improve error handling.The new error variants enhance the error handling capabilities for different scenarios:
SerdeJsonErrorwith the#[from]attribute enables automatic conversion fromserde_json::ErrorCustomErrorallows for flexible custom error messagesNoAvailableQuerierprovides a specific error for when no queriers are availableThis aligns well with the broader changes in the PR to unify node metadata retrieval.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/handlers/http/about.rs(1 hunks)src/handlers/http/query.rs(2 hunks)src/parseable/mod.rs(9 hunks)src/query/mod.rs(1 hunks)src/query/stream_schema_provider.rs(1 hunks)src/utils/arrow/flight.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/parseable/mod.rs (3)
src/handlers/http/modal/mod.rs (1)
load(276-297)src/option.rs (2)
mode(128-137)from(61-72)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
src/handlers/http/query.rs (1)
src/storage/object_storage.rs (6)
serde_json(439-439)serde_json(514-514)serde_json(541-541)serde_json(581-581)serde_json(607-607)serde_json(684-684)
src/utils/arrow/flight.rs (1)
src/query/stream_schema_provider.rs (1)
is_within_staging_window(784-806)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (15)
src/utils/arrow/flight.rs (1)
136-137: Expanded mode check to include PrismThe condition has been broadened to include
Mode::PrismalongsideMode::Query, ensuring that the function can return true for both modes when within staging window.src/handlers/http/about.rs (1)
66-67: Same staging handling for both Query and Prism modesThe condition has been updated to use empty staging string for both
Mode::Queryand the newMode::Prism, maintaining consistent behavior for these operational modes.src/query/mod.rs (1)
470-470: Expanded manifest list retrieval to include Prism modeThis change extends the special manifest retrieval logic to also apply when in
Mode::Prism, ensuring consistent manifest handling between Query and Prism modes.src/query/stream_schema_provider.rs (1)
506-506: Consistent snapshot population for Prism modeModified the condition to handle
Mode::Prismthe same way asMode::Querywhen populating the merged snapshot, ensuring that both modes share the same stream.json file retrieval logic.src/handlers/http/query.rs (4)
175-175: Support for Prism mode added to update_schema_when_distributedThe check now includes
Mode::PrismalongsideMode::Queryfor schema updates, ensuring consistency with the new mode being introduced across the system.
324-325: Added SerdeJsonError for better error handlingUsing the
#[from]attribute for automatic error conversion fromserde_json::Erroris a good practice for ergonomic error handling.
326-327: Added CustomError for dynamic error messagesThis provides flexibility to create custom error messages at runtime, enhancing error reporting capabilities.
328-329: Added NoAvailableQuerier error variantThis specific error variant clearly indicates when no querier nodes are available, which aligns with the new querier metadata functionality.
src/parseable/mod.rs (7)
50-53: Added necessary imports for node metadata supportThe imports of
NodeTypeandQuerierMetadataappropriately support the new unified node metadata approach.
134-135: Added querier_metadata field to the Parseable structThis addition properly follows the same pattern as existing
ingestor_metadataandindexer_metadatafields, providing consistency in the code structure.
166-172: Initialize querier_metadata in constructor when mode is QueryThis initialization follows the same pattern as the existing metadata initialization, maintaining code consistency.
209-209: Replaced get_ingestor_id() with more generic get_node_id()This change is part of the refactoring towards a more unified approach for node identification, which improves code consistency and maintainability.
Also applies to: 387-387, 692-692
293-326: Improved store_metadata implementationThe refactored method uses a single match statement to determine the appropriate metadata reference based on mode, which simplifies the code and reduces redundancy. The implementation properly handles querier metadata alongside ingestor and indexer metadata.
525-526: Mode check for stream_in_storage_only_for_query_nodeThe updated condition now correctly checks if the mode is either Query or Prism, ensuring proper handling of streams in both modes.
286-286: Added Prism to get_server_mode_stringThis ensures that the server mode string is properly reported when running in Prism mode.
src/parseable/mod.rs
Outdated
| ) | ||
| } | ||
|
|
||
| /// Checks for the stream in memory, or loads it from storage when in distributed mode | ||
| pub async fn check_or_load_stream(&self, stream_name: &str) -> bool { | ||
| !self.streams.contains(stream_name) | ||
| && (self.options.mode != Mode::Query | ||
| || self.options.mode != Mode::Prism |
There was a problem hiding this comment.
Logic error in check_or_load_stream condition
The condition will always evaluate to true, potentially causing unexpected behavior.
The current condition self.options.mode != Mode::Query || self.options.mode != Mode::Prism will always be true because self.options.mode cannot be both modes simultaneously. To check if the mode is neither Query nor Prism, use:
- && (self.options.mode != Mode::Query
- || self.options.mode != Mode::Prism
+ && (self.options.mode != Mode::Query
+ && self.options.mode != Mode::Prism📝 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.
| || self.options.mode != Mode::Prism | |
| && (self.options.mode != Mode::Query | |
| && self.options.mode != Mode::Prism |
nikhilsinhaparseable
force-pushed
the
add-querier-json
branch
from
6516873 to
0a85484
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/prism/logstream/mod.rs (1)
155-157: Same check for stream existence
This mirrors the behavior in line 89. The negation ensures we only proceed when the stream is found or loaded.
🧹 Nitpick comments (1)
src/parseable/mod.rs (1)
215-223: Clarifycheck_or_load_streamreturn value
The boolean return can be slightly ambiguous; consider documenting thattruemeans the stream is found or successfully loaded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parseable/mod.rs(9 hunks)src/prism/logstream/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/prism/logstream/mod.rs (1)
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-04-03T09:55:33.399Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
🧬 Code Graph Analysis (1)
src/parseable/mod.rs (3)
src/handlers/http/modal/mod.rs (1)
load(276-297)src/option.rs (2)
mode(128-137)from(61-72)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (9)
src/prism/logstream/mod.rs (2)
89-91: Ensure consistent handling of non-existent streams
Returning an error whencheck_or_load_streamfails is consistent with blocking invalid stream access. This logic looks correct for modes where the stream must be loaded.
292-294: Skip missing streams gracefully
ReturningOk(None)cleanly omits nonexistent streams. This aligns with ignoring or skipping streams that cannot be loaded rather than causing a hard error.src/parseable/mod.rs (7)
50-53: Imported additional metadata types
These imports forQuerierMetadata,NodeType, etc. appropriately support the new Prism/Query features.
133-134: Newquerier_metadatafield
Introducingquerier_metadatato store querying-related state neatly follows the pattern used for ingestor/indexer metadata.
151-155: Conditional load: Ingestor metadata
LoadingIngestorMetadataonly when inMode::Ingesthelps keep the metadata usage focused.
159-165: Conditional load: Indexer metadata
Similarly, restricting loading ofIndexerMetadatatoMode::Indexavoids unnecessary metadata overhead in other modes.
166-172: Conditional load: Querier metadata
This matches how ingestor and indexer metadata are handled, ensuring each mode has its dedicated metadata.
180-180: Addedquerier_metadatato struct
This finalizes the struct initialization for the newquerier_metadatafield.
525-529: Extended support for Prism mode
Allowingcreate_stream_and_schema_from_storageto run under bothMode::QueryandMode::Prismkeeps them in parity.
nikhilsinhaparseable
force-pushed
the
add-querier-json
branch
2 times, most recently
from
ed03452 to
d1b1f30
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/handlers/http/cluster/mod.rs (1)
557-563: Enhanced cluster info retrieval with support for queriers.The implementation now concurrently fetches metadata for all three node types (querier, ingestor, and indexer) using
future::join3, which is an excellent optimization. However, the error messages still refer specifically to node types rather than using a more generic message.Consider making the error messages more generic:
- error!("Fatal: failed to get querier info: {:?}", err); + error!("Fatal: failed to get node info for queriers: {:?}", err);Also applies to: 565-571
src/handlers/http/modal/mod.rs (4)
61-61: Updated default version from "v3" to "v4".The version update likely corresponds to the new metadata structure that includes node types. Consider adding a comment explaining what this version change represents.
279-340: Enhanced metadata loading with node type support.The
loadmethod has been properly updated to handle different node types, with appropriate helper methods for creating and updating metadata. Good separation of concerns between loading existing metadata and creating new metadata.The
load_from_stagingmethod has clear error handling but could benefit from more detailed error reporting:- error!("Failed to extract {} metadata: {}", node_type_str, e); + error!("Failed to extract {} metadata from file {}: {}", + node_type_str, path.display(), e);
423-467: Implemented version migration logic.The
from_bytesmethod now includes robust migration logic for transitioning from v3 to v4 metadata format, including field renaming and structure updates.The migration logic handles ingestor and indexer but doesn't explicitly handle querier. Consider adding a comment explaining if this is intentional because queriers are new in v4:
if !migrate_legacy_id(&mut json, "ingestor_id", "ingestor") { - migrate_legacy_id(&mut json, "indexer_id", "indexer"); + if !migrate_legacy_id(&mut json, "indexer_id", "indexer") { + // Queriers are new in v4, so no migration needed for them + } }
541-600: Updated tests for the new metadata structure.Tests have been properly updated to include the node type parameter and use the new metadata structure.
Consider adding specific tests for the querier and prism node types to ensure complete coverage of the new functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/handlers/http/cluster/mod.rs(13 hunks)src/handlers/http/modal/mod.rs(10 hunks)src/option.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/option.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(40-44)src/parseable/mod.rs (2)
new(145-184)storage(261-263)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
🔇 Additional comments (16)
src/handlers/http/cluster/mod.rs (8)
57-57: Updated node metadata imports to support querier and prism nodes.Good addition of QuerierMetadata to the imports along with the generic NodeMetadata type, aligning with the new multi-node architecture.
76-79: Consistent refactoring to use generic node info retrieval.The replacement of the specific
get_ingestor_infowith the more genericget_node_info("ingestor")improves code maintainability and follows a consistent pattern throughout the file. The explicit return type declaration asVec<NodeMetadata>clarifies the expected result.
543-555: Added node type to cluster info creation.Good enhancement of the self_info creation to include the node type from the options, maintaining consistency with the new node type architecture.
589-594: Complete integration of querier nodes in cluster info.Properly extends the pattern of fetching node information concurrently to include queriers, maintaining the parallel processing approach consistently across all node types.
Also applies to: 597-601
698-714: Implemented generic node info retrieval function.This is a well-designed generic function that replaces specific functions for different node types. The type parameter
T: Metadata + DeserializeOwnedproperly constrains the return type while allowing flexibility. Good use of owned copies of the prefix string to avoid lifetime issues in the closure.
735-736: Added querier metadata removal to node removal process.Properly extends the node removal functionality to handle querier metadata, maintaining consistency with other node types. The conditional check correctly includes the querier removal status.
Also applies to: 738-738
849-851: Added check for empty node lists.Good optimization to avoid unnecessary processing when the node list is empty. This improves performance and helps avoid potential errors when dealing with empty collections.
874-886: Complete integration of querier metrics in cluster metrics.The implementation correctly extends the pattern of fetching and handling metrics for all node types, maintaining consistency throughout the codebase.
Also applies to: 897-911
src/handlers/http/modal/mod.rs (8)
202-239: Added NodeType enum with comprehensive functionality.Well-designed enum that represents different node types with appropriate helper methods:
as_str()for string conversionto_mode()for mode conversion- Display implementation for easy formatting
The inclusion of the
PrismandAlltypes supports the expanded operational modes.
242-251: Refactored to generic NodeMetadata structure.Excellent replacement of specific metadata types with a generic structure that includes a
node_typefield. This consolidation improves code maintainability and follows the DRY principle.
342-347: Added helper method for metadata file validation.Good extraction of file validation logic into a separate method that checks if a file corresponds to a specific node type.
349-378: Updated metadata with proper node type handling.The
update_metadatamethod effectively updates all relevant fields when configuration changes and properly sets the node type. Good use of logging to track changes.
380-408: Extracted metadata creation into a dedicated method.Good separation of concerns by moving metadata creation logic into its own method. This improves code readability and maintainability.
499-501: Updated file path format to include node type.The file naming convention now includes the node type, which provides better organization and clarity in the staging directory.
509-532: Updated Metadata trait with node type method.The Metadata trait has been properly extended to include a
node_typemethod, maintaining consistency with the new architecture.
534-537: Added type aliases for backward compatibility.Good practice to provide type aliases for backward compatibility with existing code. This allows for a smoother transition to the new structure.
nikhilsinhaparseable
force-pushed
the
add-querier-json
branch
from
2109bcc to
4e7705d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/parseable/mod.rs (1)
250-258:⚠️ Potential issueFix logical error in mode checking condition.
The condition logic has been corrected to check if the mode is either
QueryorPrismbefore attempting to create a stream from storage. This ensures proper behavior for both query and prism modes.
🧹 Nitpick comments (1)
src/analytics.rs (1)
114-135: Consider reducing code duplication in liveness checking.The code for checking liveness and updating counters is duplicated between indexers and queries. Consider refactoring this into a helper function.
- let indexer_infos: Vec<NodeMetadata> = cluster::get_node_info("indexer").await?; - for indexer in indexer_infos { - if check_liveness(&indexer.domain_name).await { - active_indexers += 1; - } else { - inactive_indexers += 1; - } - } - - let query_infos: Vec<NodeMetadata> = cluster::get_node_info("query").await?; - for query in query_infos { - if check_liveness(&query.domain_name).await { - active_queries += 1; - } else { - inactive_queries += 1; - } - } + async fn count_active_nodes(node_type: &str) -> anyhow::Result<(u64, u64)> { + let node_infos: Vec<NodeMetadata> = cluster::get_node_info(node_type).await?; + let mut active = 0; + let mut inactive = 0; + for node in node_infos { + if check_liveness(&node.domain_name).await { + active += 1; + } else { + inactive += 1; + } + } + Ok((active, inactive)) + } + + let (active_indexers, inactive_indexers) = count_active_nodes("indexer").await?; + let (active_queries, inactive_queries) = count_active_nodes("query").await?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/analytics.rs(5 hunks)src/catalog/mod.rs(4 hunks)src/cli.rs(2 hunks)src/handlers/airplane.rs(2 hunks)src/handlers/http/about.rs(1 hunks)src/handlers/http/cluster/mod.rs(13 hunks)src/handlers/http/cluster/utils.rs(4 hunks)src/handlers/http/logstream.rs(9 hunks)src/handlers/http/middleware.rs(1 hunks)src/handlers/http/mod.rs(2 hunks)src/handlers/http/modal/mod.rs(10 hunks)src/handlers/http/modal/query/querier_logstream.rs(2 hunks)src/handlers/http/query.rs(2 hunks)src/logstream/mod.rs(2 hunks)src/main.rs(1 hunks)src/option.rs(3 hunks)src/parseable/mod.rs(8 hunks)src/prism/logstream/mod.rs(3 hunks)src/query/mod.rs(1 hunks)src/query/stream_schema_provider.rs(1 hunks)src/storage/mod.rs(1 hunks)src/storage/object_storage.rs(2 hunks)src/storage/store_metadata.rs(4 hunks)src/utils/arrow/flight.rs(1 hunks)src/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- src/query/mod.rs
- src/utils/arrow/flight.rs
- src/main.rs
- src/handlers/airplane.rs
- src/option.rs
- src/storage/object_storage.rs
- src/prism/logstream/mod.rs
- src/handlers/http/logstream.rs
- src/handlers/http/middleware.rs
- src/logstream/mod.rs
- src/handlers/http/about.rs
- src/handlers/http/mod.rs
- src/handlers/http/query.rs
- src/storage/mod.rs
- src/catalog/mod.rs
- src/storage/store_metadata.rs
- src/handlers/http/modal/query/querier_logstream.rs
- src/handlers/http/cluster/utils.rs
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/modal/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1288
File: src/handlers/http/modal/mod.rs:279-301
Timestamp: 2025-04-07T13:23:10.092Z
Learning: For critical operations like writing metadata to disk in NodeMetadata::put_on_disk(), it's preferred to let exceptions propagate (using expect/unwrap) rather than trying to recover with fallback mechanisms, as the failure indicates a fundamental system issue that needs immediate attention.
🧬 Code Graph Analysis (4)
src/utils/mod.rs (1)
src/handlers/http/modal/mod.rs (1)
get_node_id(410-412)
src/analytics.rs (2)
src/handlers/http/cluster/mod.rs (1)
get_node_info(691-707)src/handlers/http/cluster/utils.rs (1)
check_liveness(178-198)
src/parseable/mod.rs (5)
src/parseable/streams.rs (1)
new(107-124)src/parseable/staging/reader.rs (1)
new(186-204)src/parseable/staging/writer.rs (1)
default(119-126)src/storage/object_storage.rs (7)
get_metadata(464-480)serde_json(439-439)serde_json(514-514)serde_json(541-541)serde_json(581-581)serde_json(607-607)serde_json(684-684)src/option.rs (2)
mode(140-149)from(73-84)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(40-44)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- 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
🔇 Additional comments (34)
src/query/stream_schema_provider.rs (1)
506-506: Expanded conditional logic to include Prism mode.The condition now handles both
Mode::QueryandMode::Prismfor constructing and retrieving objects fromglob_storage. This ensures consistent behavior between Query and Prism operational modes.src/cli.rs (2)
331-337: Good addition of the querier endpoint configuration.The new
querier_endpointfield follows the established pattern of other endpoint configurations and includes appropriate documentation.
481-494: Properly implemented URL handling for querier mode.The implementation for
Mode::Queryfollows the same pattern as other modes, ensuring consistency in error handling and URL construction.src/utils/mod.rs (1)
40-44: Good refactoring to a generic node ID generator.Renaming from
get_ingestor_idtoget_node_idimproves code clarity and aligns with the addition of the Prism mode. This generic approach removes the need for mode-specific ID generation functions.src/analytics.rs (3)
78-81: Added comprehensive node tracking fields.The addition of fields to track both active and inactive indexers and queries enhances the analytics capabilities.
257-257: Expanded conditional to include Prism mode.The condition now handles both
Mode::QueryandMode::Prismfor fetching ingestor metrics, ensuring consistent behavior between these operational modes.
261-261: Improved error handling with proper type annotation.The code now uses explicit type annotation (
Vec<NodeMetadata>) and the?operator for error propagation instead of usingunwrap(), which is a more robust approach.src/handlers/http/cluster/mod.rs (11)
57-57: Updated modal imports to include new metadata types.The import now includes
NodeMetadataalong with the existing metadata types. This is part of the effort to unify all node types under a common interface.
76-79: Updated to use the genericget_node_infofunction instead of specific functions.This change switches from using a specialized ingestor info function to a more generic approach that returns
Vec<NodeMetadata>. This improves code reusability and maintainability by handling all node types with a single function.
542-558: Added querier metadata handling toget_cluster_info.The function now retrieves metadata for queriers, ingestors, and indexers concurrently using
future::join3, enhancing the functionality to support the new Prism mode.
575-579: Added self metadata handling to track current node information.This addition allows the node to provide information about itself in the cluster info, making the cluster view more comprehensive.
582-587: Updatedfetch_nodes_infocalls to include querier and self metadata.The function now fetches information for all node types including the node itself, providing a complete picture of the cluster.
592-594: Extended info collection to include self and querier info.Now includes metadata for all node types in the response, ensuring comprehensive cluster information.
691-707: Implemented genericget_node_infofunction.This is a significant improvement in the API design. The function now accepts a type parameter
Tthat must implementMetadataandDeserializeOwnedtraits, and a prefix string to determine the type of node to query. This allows a single function to handle metadata retrieval for any node type.
727-731: Added querier metadata handling in node removal.The
remove_nodefunction now handles querier metadata removal, ensuring that all node types can be properly managed within the cluster.
842-844: Added empty check to prevent unnecessary processing.This optimization returns an empty vector when there are no nodes to process, avoiding unnecessary operations and potential errors with empty inputs.
867-873: Added querier data retrieval tofetch_cluster_metrics.The function now retrieves metrics data for queriers alongside ingestors and indexers, supporting the expanded node type system.
874-878: Added self metadata handling in metrics collection.Now includes the current node's metrics data in the collection, providing more comprehensive metrics reporting.
src/parseable/mod.rs (7)
133-136: Added new metadata fields for Prism and Querier modes.These new fields enable the Parseable struct to handle additional operational modes, enhancing the system's flexibility and supporting the Prism operational mode.
152-159: Added Prism metadata initialization in thenewmethod.This code initializes the new
prism_metadatafield when the mode isMode::Prism, supporting the new operational mode.
176-183: Added querier metadata initialization in thenewmethod.Initializes the
querier_metadatafield when the mode isMode::Query, completing the support for all operational modes.
197-218: Added method to retrieve current node metadata.The
get_metadatamethod provides a unified way to access the node's metadata based on its current mode, simplifying code that needs to interact with the node regardless of its type.
239-243: Updated to useget_node_id()instead of type-specific methods.This change is part of the consistent refactoring to use unified node ID retrieval methods, enhancing code consistency.
327-361: Refactoredstore_metadatato handle all metadata types.This is a significant improvement that uses a single match statement to determine the appropriate metadata reference based on the mode. The approach is more maintainable and reduces code duplication.
559-565: Updated stream checking condition to include Prism mode.The condition now checks for both
Mode::QueryandMode::Prismwhen determining if a stream exists in storage only. This ensures that streams are handled consistently across all relevant modes.src/handlers/http/modal/mod.rs (9)
61-61: Updated DEFAULT_VERSION to v4.This change reflects the introduction of the new metadata structure that includes support for all node types.
202-239: Added NodeType enum to represent different node types.This is a well-designed enum that represents all node types in the system with helpful conversion methods to string representations and Mode values. The implementation of Display is a nice touch for easy debugging and logging.
241-251: Generalized IngestorMetadata to NodeMetadata.The metadata structure has been renamed and refactored to work with all node types, with the addition of a
node_typefield to distinguish between them.
280-301: Enhanced metadata loading to support all node types.The load method now accepts a node_type parameter and uses it to determine how to handle metadata loading and creation. This supports the unified approach to node metadata.
303-340: Improved metadata loading from staging directory.The method now checks specifically for files matching the node type, providing better encapsulation and separation of concerns.
422-467: Added metadata version migration support.The code now handles migration from v3 to v4 metadata format, properly updating field names and ensuring backward compatibility. This is crucial for seamless updates.
478-491: Enhanced metadata migration to preserve node type.When migrating metadata, the code now ensures that the node type is preserved, maintaining consistency across the system.
509-527: Updated Metadata trait to include node_type accessor.The Metadata trait now includes a method for retrieving the node type, ensuring that all implementing types can properly identify themselves.
534-538: Added type aliases for backward compatibility.These aliases ensure that existing code can continue to use the previous type names while the codebase transitions to the new unified approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli.rs (1)
450-552: Consider refactoring to reduce code duplication.The
get_urlmethod has significant code duplication across the different mode cases. Since you're already extending this method, it might be a good opportunity to refactor it to reduce duplication, as noted in the TODO comment on line 450.pub fn get_url(&self, mode: Mode) -> Url { - let (endpoint, env_var) = match mode { - Mode::Ingest => { - if self.ingestor_endpoint.is_empty() { - return format!( - "{}://{}", - self.get_scheme(), - self.address - ) - .parse::<Url>() // if the value was improperly set, this will panic before hand - .unwrap_or_else(|err| { - panic!("{err}, failed to parse `{}` as Url. Please set the environment variable `P_ADDR` to `<ip address>:<port>` without the scheme (e.g., 192.168.1.1:8000). Please refer to the documentation: https://logg.ing/env for more details.", self.address) - }); - } - (&self.ingestor_endpoint, "P_INGESTOR_ENDPOINT") - } - Mode::Index => { - if self.indexer_endpoint.is_empty() { - return format!( - "{}://{}", - self.get_scheme(), - self.address - ) - .parse::<Url>() // if the value was improperly set, this will panic before hand - .unwrap_or_else(|err| { - panic!("{err}, failed to parse `{}` as Url. Please set the environment variable `P_ADDR` to `<ip address>:<port>` without the scheme (e.g., 192.168.1.1:8000). Please refer to the documentation: https://logg.ing/env for more details.", self.address) - }); - } - (&self.indexer_endpoint, "P_INDEXER_ENDPOINT") - } - Mode::Query => { - if self.querier_endpoint.is_empty() { - return format!( - "{}://{}", - self.get_scheme(), - self.address - ) - .parse::<Url>() // if the value was improperly set, this will panic before hand - .unwrap_or_else(|err| { - panic!("{err}, failed to parse `{}` as Url. Please set the environment variable `P_ADDR` to `<ip address>:<port>` without the scheme (e.g., 192.168.1.1:8000). Please refer to the documentation: https://logg.ing/env for more details.", self.address) - }); - } - (&self.querier_endpoint, "P_QUERIER_ENDPOINT") - } - _ => { - return format!( - "{}://{}", - self.get_scheme(), - self.address - ) - .parse::<Url>() // if the value was improperly set, this will panic before hand - .unwrap_or_else(|err| { - panic!("{err}, failed to parse `{}` as Url. Please set the environment variable `P_ADDR` to `<ip address>:<port>` without the scheme (e.g., 192.168.1.1:8000). Please refer to the documentation: https://logg.ing/env for more details.", self.address) - }); - } + // Helper function to generate default URL + let default_url = || { + format!("{}://{}", self.get_scheme(), self.address) + .parse::<Url>() + .unwrap_or_else(|err| { + panic!("{err}, failed to parse `{}` as Url. Please set the environment variable `P_ADDR` to `<ip address>:<port>` without the scheme (e.g., 192.168.1.1:8000). Please refer to the documentation: https://logg.ing/env for more details.", self.address) + }) + }; + + let (endpoint, env_var) = match mode { + Mode::Ingest => { + if self.ingestor_endpoint.is_empty() { + return default_url(); + } + (&self.ingestor_endpoint, "P_INGESTOR_ENDPOINT") + } + Mode::Index => { + if self.indexer_endpoint.is_empty() { + return default_url(); + } + (&self.indexer_endpoint, "P_INDEXER_ENDPOINT") + } + Mode::Query => { + if self.querier_endpoint.is_empty() { + return default_url(); + } + (&self.querier_endpoint, "P_QUERIER_ENDPOINT") + } + _ => { + return default_url(); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (3)
src/cli.rs (3)
331-337: LGTM: Newquerier_endpointfield added correctly.The addition of the
querier_endpointfield follows the same pattern as the existing endpoint fields, with appropriate CLI argument configuration and help text.
481-494: LGTM: Mode::Query handler implementation looks good.The implementation for the new
Mode::Querycase correctly follows the same pattern as the existing mode handlers, with proper URL construction and error handling.
495-505:✅ Verification successful
Verify the catch-all case behavior.
The catch-all case (
_) previously might have had different behavior. Ensure that all other modes (like the newPrismmode mentioned in the PR summary) function correctly with this implementation.
🏁 Script executed:
#!/bin/bash # Find all references to Mode variants to ensure all are handled correctly rg "Mode::" -A 3 -B 3Length of output: 25540
Verified: Catch-All Case in
get_urlWorks as Expected
- The search confirms that explicit branches exist for
Mode::Ingest,Mode::Index, andMode::Query.- For modes not explicitly handled – including the new
Mode::Prismand others – the implementation falls back to constructing the URL usingself.get_scheme()andself.address.- This default behavior appears consistent with the current design: if no dedicated endpoint is configured (as with Prism), the catch-all branch provides a valid fallback.
- If a dedicated endpoint for Prism mode is needed in the future, consider adding an explicit branch and updating the documentation accordingly.
nikhilsinhaparseable
force-pushed
the
add-querier-json
branch
from
cb07dde to
48b121c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/handlers/http/modal/mod.rs (2)
304-340: Error handling in load_from_staging could be improved.While the function correctly handles file reading errors, the error message when metadata extraction fails could be more informative.
match Self::from_bytes(&bytes, options.flight_port) { Ok(meta) => return Some(meta), Err(e) => { - error!("Failed to extract {} metadata: {}", node_type_str, e); + error!("Failed to extract {} metadata from {}: {}", + node_type_str, path.display(), e); return None; } }
423-467: Comprehensive migration logic with room for improvement.The migration logic is thorough in handling the transition from v3 to v4 metadata format, but could benefit from additional documentation.
Consider adding a few more comments explaining the overall migration strategy, particularly around the nested migration_legacy_id function and why both "ingestor_id" and "indexer_id" are checked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/analytics.rs(5 hunks)src/catalog/mod.rs(4 hunks)src/cli.rs(2 hunks)src/handlers/airplane.rs(2 hunks)src/handlers/http/about.rs(1 hunks)src/handlers/http/cluster/mod.rs(13 hunks)src/handlers/http/cluster/utils.rs(4 hunks)src/handlers/http/logstream.rs(9 hunks)src/handlers/http/middleware.rs(1 hunks)src/handlers/http/mod.rs(2 hunks)src/handlers/http/modal/mod.rs(10 hunks)src/handlers/http/modal/query/querier_logstream.rs(2 hunks)src/handlers/http/query.rs(2 hunks)src/logstream/mod.rs(2 hunks)src/main.rs(1 hunks)src/option.rs(3 hunks)src/parseable/mod.rs(8 hunks)src/prism/logstream/mod.rs(3 hunks)src/query/mod.rs(1 hunks)src/query/stream_schema_provider.rs(1 hunks)src/storage/mod.rs(1 hunks)src/storage/object_storage.rs(2 hunks)src/storage/store_metadata.rs(4 hunks)src/utils/arrow/flight.rs(1 hunks)src/utils/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- src/logstream/mod.rs
- src/handlers/http/about.rs
- src/storage/store_metadata.rs
- src/storage/mod.rs
- src/query/stream_schema_provider.rs
- src/handlers/airplane.rs
- src/query/mod.rs
- src/handlers/http/modal/query/querier_logstream.rs
- src/utils/mod.rs
- src/handlers/http/middleware.rs
- src/handlers/http/logstream.rs
- src/storage/object_storage.rs
- src/main.rs
- src/prism/logstream/mod.rs
- src/catalog/mod.rs
- src/option.rs
- src/handlers/http/mod.rs
- src/utils/arrow/flight.rs
- src/handlers/http/cluster/utils.rs
- src/handlers/http/query.rs
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/modal/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1288
File: src/handlers/http/modal/mod.rs:279-301
Timestamp: 2025-04-07T13:23:10.092Z
Learning: For critical operations like writing metadata to disk in NodeMetadata::put_on_disk(), it's preferred to let exceptions propagate (using expect/unwrap) rather than trying to recover with fallback mechanisms, as the failure indicates a fundamental system issue that needs immediate attention.
🧬 Code Graph Analysis (2)
src/cli.rs (4)
src/option.rs (1)
mode(140-149)src/storage/azure_blob.rs (1)
get_endpoint(192-194)src/storage/localfs.rs (1)
get_endpoint(83-85)src/storage/s3.rs (1)
get_endpoint(322-324)
src/handlers/http/modal/mod.rs (2)
src/utils/mod.rs (1)
get_node_id(40-44)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (34)
src/cli.rs (4)
331-337: LGTM: Good addition ofquerier_endpointthat follows existing patterns.The addition of the
querier_endpointfield to theOptionsstruct follows the same pattern as the existingingestor_endpointandindexer_endpointfields, maintaining consistency in the API design.
450-456: Good refactoring of endpoint handling logic.The
get_urlmethod has been nicely refactored to use theget_endpointhelper function, making the code more maintainable by avoiding duplication across different mode handlers.
461-473: Well-implemented endpoint validation.The
get_endpointhelper function provides consistent validation for all endpoint types, with clear error messages that guide users to the proper documentation.
475-489: Solid modular URL parsing implementation.The URL parsing logic has been effectively decomposed into three separate methods:
parse_endpoint- Handles input parsing and validationresolve_env_var- Processes environment variable referencesbuild_url- Constructs the final URL with appropriate error handlingThis modular approach improves maintainability and makes the code easier to understand and test.
Also applies to: 491-511, 513-522
src/analytics.rs (4)
78-81: Good extension of metrics to include indexers and queries.These new fields in the
Reportstruct maintain consistency with the existing pattern for ingestors, providing a complete view of all node types in the system.
114-135: Well-implemented node metrics collection.The implementation for collecting indexer and query metrics follows the same pattern as for ingestors, which maintains consistency. The code also properly handles liveness checks and counter updates.
151-154: Good update of the report construction.The updated
Ok(Self{...})return statement correctly includes the newly collected metrics for indexers and queries.
257-257: Properly extended condition to include Prism mode.The condition has been updated to include the new Prism mode for fetching ingestor metrics, ensuring consistent behavior across operational modes.
src/handlers/http/cluster/mod.rs (5)
76-76: Consistent updates to use generic node info function.All calls to
get_ingestor_infohave been properly updated to use the new genericget_node_info("ingestor")function, ensuring consistent behavior throughout the code.Also applies to: 124-124, 174-174, 221-221, 279-279, 327-327
542-558: Well-implemented concurrent fetching of node information.The
get_cluster_infofunction now usesfuture::join3to concurrently fetch information for queriers, ingestors, and indexers, which improves performance and follows good asynchronous programming practices.
664-666: Good defensive programming with empty node list check.Adding a check for empty node lists in
fetch_nodes_infoprevents unnecessary processing and potential errors when no nodes are provided, improving the robustness of the code.
691-707: Excellent generalization with the newget_node_infofunction.The new generic function provides a flexible way to retrieve metadata for any node type, improving code reuse and maintainability. The type constraint
T: Metadata + DeserializeOwnedensures type safety while allowing flexibility.
727-730: Complete handling for querier metadata in node removal.The
remove_nodefunction has been properly extended to handle querier metadata, ensuring that all node types can be managed consistently.src/parseable/mod.rs (6)
134-136: Good addition of metadata fields for new node types.The new fields for prism and querier metadata follow the same pattern as the existing fields for ingestors and indexers, maintaining consistency in the API design.
152-159: Well-structured initialization of new metadata types.The initialization of prism and querier metadata follows the same pattern as for ingestors and indexers, ensuring consistent behavior across all node types.
Also applies to: 176-182
198-219: Excellent implementation ofget_metadatamethod.The new method provides a clean, generic way to retrieve metadata for the current node based on its mode. It properly handles all node types and follows good error handling practices by returning
Option<NodeMetadata>.
328-362: Well-refactoredstore_metadatamethod.The method now uses a single match statement to determine which metadata reference to use based on the mode, which simplifies the logic and reduces redundancy. The error handling is also well implemented.
254-254: Properly extended condition forcheck_or_load_stream.The condition now includes the Prism mode along with Query mode for checking or loading a stream, ensuring consistent behavior across operational modes.
562-562: Same pattern correctly applied increate_update_stream.The condition for checking if a stream exists in storage has been extended to include the Prism mode, maintaining consistency with other parts of the code.
src/handlers/http/modal/mod.rs (15)
19-19: Good additions to imports for expanded functionality.The added imports support the new NodeType enum and node ID functionality, maintaining clean code organization.
Also applies to: 45-45
61-61: Version increment reflects the metadata schema update.Updating DEFAULT_VERSION from "v3" to "v4" properly signals the format changes introduced with the NodeType-based architecture.
202-239: Well-structured NodeType enum with comprehensive implementations.The NodeType enum is well-designed with appropriate methods for string conversion, mode mapping, and formatting. The #[serde(rename_all = "lowercase")] annotation ensures proper serialization/deserialization.
241-251: Good refactoring from specific to generic metadata structure.The change from IngestorMetadata to NodeMetadata with a node_type field is a good architectural improvement that allows for a unified approach to handling different node types.
279-301: Appropriate error handling for critical disk operations.The implementation correctly uses expect() for critical disk operations in accordance with the project's preference. As noted in the retrieved learning from the PR author, failures here represent fundamental system issues that should not be silently handled.
342-347: Simple and effective file name validation.The is_valid_metadata_file method provides a clean approach to validating metadata files based on node type.
349-378: Well-structured metadata update method.The update_metadata method properly handles the update logic with appropriate logging at different levels (info for domain/port changes, warn for credential changes).
380-400: Clean factory method for creating metadata.The create_new_metadata method serves as a good factory method that encapsulates the construction logic.
402-412: Good helper methods for token generation and ID retrieval.These utility methods are clear and focused on single responsibilities.
414-420: Good file path construction for different node types.The file_path method effectively uses the node_type to dynamically construct the appropriate file path.
469-491: Good adjustment of migrate method to handle node types.The migrate method now properly sets the node_type field during migration.
493-506: Updated documentation and implementation for put_on_disk.The function properly uses the node_type in the file name format.
509-532: Updated Metadata trait implementation for NodeType.The trait implementation has been correctly updated to include the node_type accessor.
534-538: Good use of type aliases for backward compatibility.The type aliases provide a smooth transition path for existing code that uses the specific metadata types.
546-601: Tests properly updated to use the new NodeType enum.The tests have been adjusted to work with the new structure and format changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/handlers/http/modal/mod.rs (1)
330-337: Consider improving error handling in metadata extractionWhen extracting metadata from bytes in the
load_from_stagingmethod, the current implementation returnsNonewith only an error log message if parsing fails. This makes it difficult to diagnose issues with corrupted metadata files.Consider differentiating between different types of errors:
-let bytes = std::fs::read(&path).expect("File should be present"); -match Self::from_bytes(&bytes, options.flight_port) { - Ok(meta) => return Some(meta), - Err(e) => { - error!("Failed to extract {} metadata: {}", node_type_str, e); - return None; - } -} +match std::fs::read(&path) { + Ok(bytes) => match Self::from_bytes(&bytes, options.flight_port) { + Ok(meta) => return Some(meta), + Err(e) => { + error!("Failed to parse {} metadata from {}: {}", + node_type_str, path.display(), e); + return None; + } + }, + Err(e) => { + error!("Failed to read {} metadata file {}: {}", + node_type_str, path.display(), e); + return None; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/analytics.rs(5 hunks)src/catalog/mod.rs(4 hunks)src/cli.rs(2 hunks)src/handlers/airplane.rs(2 hunks)src/handlers/http/about.rs(1 hunks)src/handlers/http/cluster/mod.rs(14 hunks)src/handlers/http/mod.rs(2 hunks)src/handlers/http/modal/mod.rs(10 hunks)src/handlers/http/modal/query/querier_logstream.rs(2 hunks)src/handlers/http/query.rs(2 hunks)src/parseable/mod.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/handlers/http/about.rs
- src/handlers/http/modal/query/querier_logstream.rs
- src/handlers/http/query.rs
- src/catalog/mod.rs
- src/handlers/airplane.rs
- src/handlers/http/mod.rs
🧰 Additional context used
🧠 Learnings (2)
src/handlers/http/modal/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1288
File: src/handlers/http/modal/mod.rs:279-301
Timestamp: 2025-04-07T13:23:10.092Z
Learning: For critical operations like writing metadata to disk in NodeMetadata::put_on_disk(), it's preferred to let exceptions propagate (using expect/unwrap) rather than trying to recover with fallback mechanisms, as the failure indicates a fundamental system issue that needs immediate attention.
src/parseable/mod.rs (1)
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
🧬 Code Graph Analysis (4)
src/analytics.rs (2)
src/handlers/http/cluster/mod.rs (1)
get_node_info(706-724)src/handlers/http/cluster/utils.rs (1)
check_liveness(178-198)
src/handlers/http/cluster/mod.rs (2)
src/handlers/http/modal/mod.rs (5)
new(256-278)node_type(513-513)node_type(526-528)domain_name(511-511)domain_name(518-520)src/handlers/http/cluster/utils.rs (4)
new(38-50)new(65-83)new(98-116)new(128-135)
src/handlers/http/modal/mod.rs (3)
src/utils/mod.rs (1)
get_node_id(40-44)src/handlers/http/cluster/utils.rs (4)
new(38-50)new(65-83)new(98-116)new(128-135)src/handlers/http/modal/utils/logstream_utils.rs (1)
from(42-69)
src/parseable/mod.rs (3)
src/metadata.rs (1)
new(95-130)src/parseable/staging/writer.rs (1)
default(119-126)src/option.rs (2)
mode(140-149)from(73-84)
⏰ 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 aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (35)
src/cli.rs (4)
331-337: Well-structured querier endpoint optionThe new
querier_endpointfield is properly defined with consistent patterns matching the existing endpoint fields. Good use of CLI attributes for environment variable mapping and help documentation.
453-457: Good integration of the Query mode in URL handlingThe addition of the Query mode case correctly uses the querier_endpoint to construct the URL, maintaining consistency with the existing pattern for other modes.
463-477: Clean endpoint helper method extractionGood refactoring of the endpoint handling logic into this helper method. The implementation provides proper validation and error handling for empty and invalid endpoint values.
479-496: Well-structured endpoint parsing and validationThe extracted methods for parsing, resolving, and building URLs create a clean, maintainable structure with good error handling. The comprehensive validation of inputs and helpful error messages will aid users in troubleshooting configuration issues.
Also applies to: 501-521, 524-533
src/analytics.rs (5)
78-81: Good addition of new metrics fieldsThe new fields for tracking indexers and queriers follow the same naming convention as the existing ingestor fields, maintaining consistency in the codebase.
114-139: Well-implemented node liveness trackingThis code correctly implements liveness checking for indexers and queriers, following the same pattern used for ingestors. Good use of the
get_node_infofunction to retrieve node metadata.
155-158: Properly integrated new metrics in the ReportThe new metrics are correctly added to the Report object, maintaining the established structure.
262-265: Clear mode extension for analytics reportingGood update to include both Query and Prism modes for fetching analytics reports. The comment clearly explains the purpose of this condition.
268-268: Clean transition to generic node info retrievalGood replacement of the specific ingestor info retrieval with the generic
get_node_infoapproach, which improves code flexibility and consistency.src/handlers/http/cluster/mod.rs (11)
57-59: Well-structured imports for node metadata typesGood organization of imports to include all necessary types for the node metadata handling.
78-82: Consistent refactoring to generic node info retrievalAll instances of
get_ingestor_info()have been systematically updated to useget_node_info(NodeType::Ingestor), providing better type safety and consistency throughout the codebase.Also applies to: 127-131, 178-182, 226-230, 285-289, 334-338
550-567: Well-implemented concurrent node information fetchingGood refactoring to fetch querier, ingestor, and indexer metadata concurrently using
future::join3. The error handling is robust and consistent with the established pattern.
592-597: Good implementation of self-metadata handlingThe addition of self-metadata handling in the node info fetching is well-implemented and consistent with the existing patterns.
602-604: Clean combination of node resultsGood restructuring to include querier info in the combined results.
706-724: Well-designed generic node info retrieval functionThis function is well-designed to work with any metadata type through generics. The implementation is clean, with proper handling of the node type filter and deserialization.
750-754: Complete node removal functionalityThe addition of querier metadata removal ensures that all node types are properly handled during node removal operations.
767-769: Good documentation for helper functionClear and informative documentation for the
remove_node_metadatafunction that explains its purpose and return values.
871-873: Efficient handling of empty node listsGood addition of an early return for empty node lists to avoid unnecessary processing.
899-903: Well-implemented concurrent metrics fetchingThe refactoring to fetch metrics from all node types concurrently is well-implemented and consistent with the existing patterns.
Also applies to: 912-917
939-949: Complete self and querier metrics handlingGood addition of self and querier metrics handling in the metrics collection process.
src/parseable/mod.rs (8)
133-137: Well-structured metadata fieldsThe addition of
prism_metadataandquerier_metadatafields follows the same pattern as the existing metadata fields, with clear documentation.
152-159: Clean initialization of new metadata fieldsThe initialization of the prism and querier metadata follows the same pattern as the existing metadata fields, with proper mode checking.
Also applies to: 176-182
198-219: Well-implemented metadata retrievalThe new
get_metadatamethod properly handles all node types and returns a consistently structured NodeMetadata object.
254-255: Correct mode handling in stream loadingThe condition properly checks for both Query and Prism modes, ensuring consistent behavior across operational modes.
323-323: Complete server mode string handlingThe addition of the Prism mode case in the server mode string mapping ensures all modes are properly represented.
330-335: Well-refactored metadata reference selectionGood refactoring to use a match statement for selecting the appropriate metadata reference based on the mode, which improves readability and maintainability.
337-360: Clean metadata storage implementationThe implementation properly handles the case where metadata doesn't exist and includes appropriate error handling.
564-565: Correct mode handling in stream creation/updateThe condition properly includes both Query and Prism modes when checking for stream existence in storage, ensuring consistent behavior.
src/handlers/http/modal/mod.rs (7)
61-61: Version constant updated from "v3" to "v4"The
DEFAULT_VERSIONconstant has been updated from "v3" to "v4", which indicates a breaking change in the metadata structure. This change is appropriate given the introduction of the newNodeTypeenum and the consolidation of metadata structures.
202-240: Well-designed NodeType enum implementationThe new
NodeTypeenum is well-structured with appropriate methods for string conversion, mode conversion, and display formatting. The implementation follows Rust best practices by providing a default value and implementing the Display trait.
242-252: Good consolidation of metadata with NodeMetadataThe refactoring from
IngestorMetadatatoNodeMetadatawith the addition of thenode_typefield improves code organization by consolidating multiple node-specific metadata structures into a single, flexible structure.
280-302: Improved load method with node type supportThe
loadmethod now accepts anode_typeparameter, which allows it to dynamically determine the appropriate metadata file. The method maintains the existing error handling approach as requested in previous feedback, usingexpectfor critical operations where recovery isn't possible.
427-457: Well-structured metadata migration logicThe metadata migration logic in
from_byteshandles the transition from "v3" to "v4" format well, especially the conversion from node-specific ID fields to the genericnode_idfield. The nested functionmigrate_legacy_idis a good abstraction for this task.
494-507: Improved metadata storage with node type informationThe
put_on_diskmethod now includes the node type in the file name, which makes it easier to distinguish between different node type metadata files. This is a good improvement for diagnostics and management.
535-539: Good use of type aliases for backward compatibilityThe introduction of type aliases (
IngestorMetadata,IndexerMetadata, etc.) provides backward compatibility while allowing the codebase to move forward with the consolidatedNodeMetadatastructure. This is a good approach for managing breaking changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/modal/ingest/ingestor_logstream.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/handlers/http/modal/ingest/ingestor_logstream.rs (1)
src/validator.rs (1)
stream_name(33-68)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (1)
src/handlers/http/modal/ingest/ingestor_logstream.rs (1)
19-20: Appropriate import added for filesystem operations.The addition of the
std::fsimport is necessary for the new functionality to delete local stream data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/handlers/http/modal/mod.rs (4)
243-278: Unified NodeMetadata StructCombining multiple node-type-specific metadata into a single
NodeMetadatastruct eliminates duplication and simplifies maintenance. If the parameter list keeps growing, consider a builder or a config struct pattern to improve clarity.
339-405: Avoid Potential Partial Substring MatchesThe storage filtering uses
file_name.contains(&node_type), which could, in theory, match unintended filenames if a node type name is a substring of another node type or file identifier. To reduce the risk of accidental matches, consider using a more explicit check such as checking prefixes or exact patterns:- Box::new({ - let node_type = node_type.clone(); - move |file_name| file_name.contains(&node_type) - }), + Box::new({ + let node_type = format!("{}.", node_type.clone()); + move |file_name| file_name.starts_with(&node_type) + }),
468-485: Basic Auth TokenGenerating a Base64-encoded
username:passwordtoken is convenient but not inherently secure if logs or storage are exposed. If the environment requires stronger protection, consider a hashed or token-based approach. Otherwise, this is acceptable for controlled or internal deployments.
587-643: Consider Additional Prism TestsThe updated tests confirm behavior for
Ingestormetadata. For completeness, adding test coverage for the newPrismvariant (or other node types) ensures consistent serialization and migration logic across all variants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/handlers/http/cluster/mod.rs(14 hunks)src/handlers/http/modal/ingest_server.rs(3 hunks)src/handlers/http/modal/mod.rs(8 hunks)src/handlers/http/modal/query_server.rs(2 hunks)src/parseable/mod.rs(6 hunks)src/storage/object_storage.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/storage/object_storage.rs
🧰 Additional context used
🧠 Learnings (2)
src/parseable/mod.rs (1)
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
src/handlers/http/modal/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1288
File: src/handlers/http/modal/mod.rs:279-301
Timestamp: 2025-04-07T13:23:10.092Z
Learning: For critical operations like writing metadata to disk in NodeMetadata::put_on_disk(), it's preferred to let exceptions propagate (using expect/unwrap) rather than trying to recover with fallback mechanisms, as the failure indicates a fundamental system issue that needs immediate attention.
🧬 Code Graph Analysis (1)
src/handlers/http/modal/query_server.rs (1)
src/handlers/http/modal/mod.rs (2)
load_on_init(167-200)load_node_metadata(280-306)
🪛 GitHub Actions: Lint, Test and Coverage Report
src/parseable/mod.rs
[error] 623-627: Clippy lint 'manual_map' triggered: manual implementation of Option::map. Suggestion: replace with INGESTOR_META.get().map(|ingestor_metadata| ingestor_metadata.get_node_id()). Compilation failed due to this lint error.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (38)
src/handlers/http/modal/query_server.rs (4)
35-35: New import added for OnceCellAdding
OnceCellfromtokio::syncto manage the global querier metadata state.
42-42: Updated imports to support the new metadata architectureImporting
NodeTypeand other related types to support the unified node metadata handling approach.
45-45: Added global static metadata storageCreation of a static
QUERIER_METAusingOnceCellto provide thread-safe global access to querier metadata.
103-109: Implemented querier metadata initializationThe initialization pattern using
get_or_initensures the metadata is loaded exactly once during server startup. The approach is thread-safe and handles errors appropriately.src/handlers/http/modal/ingest_server.rs (5)
30-31: Added OnceCell import for metadata managementIncluding
OnceCellfromtokio::syncto support thread-safe global state management.
32-33: Imported NodeType to support unified node metadata handlingUsing the
NodeTypeenum to identify the current node, which enables the generic node metadata approach.
50-51: Updated imports for the new metadata structureImporting
IngestorMetadatadirectly to support the unified metadata handling approach.
57-57: Created global ingestor metadata singletonThe static
INGESTOR_METAprovides global access to ingestor metadata in a thread-safe manner usingOnceCell.
104-110: Implemented thread-safe ingestor metadata initializationGood implementation of async one-time initialization for ingestor metadata using
get_or_init. This ensures the metadata is initialized exactly once with proper error handling.src/handlers/http/cluster/mod.rs (17)
57-59: Updated imports for unified node metadataImporting all necessary node metadata types to support the generic approach to node handling.
79-82: Refactored ingestor info retrievalReplaced direct ingestor info retrieval with the generic
get_node_infofunction, improving code maintainability.
127-131: Consistently refactored node info retrievalAll instances of ingestor metadata retrieval now use the generic
get_node_infoapproach, creating a consistent pattern throughout the codebase.Also applies to: 178-182, 226-230, 285-289, 334-338
550-559: Enhanced cluster information to include querier nodesThe function now concurrently fetches metadata for all node types (querier, ingestor, indexer) using
future::join3, improving performance and adding support for queriers.
560-567: Added proper error handling for querier metadataThe code correctly handles potential errors when retrieving querier metadata, maintaining consistency with the existing error handling approach.
585-586: Updated concurrent info fetching to include queriersThe code now fetches node information for all three node types in parallel, improving performance.
593-595: Added querier info to combined resultsThe cluster info now includes data from all node types, providing a complete view of the cluster.
601-603: Enhanced function documentationImproved the description of
fetch_node_infoto clarify that it calls the node's/aboutendpoint and constructs aClusterInfostruct.
667-669: Added early return for empty node listsThe function now returns early when the input node list is empty, avoiding unnecessary processing.
694-715: Created generic node info retrieval functionImplemented a type-parametric
get_node_info<T>function that retrieves metadata for any node type, promoting code reuse and maintainability.
741-745: Added querier node removal supportExtended the
remove_nodefunction to handle querier metadata removal, ensuring all node types can be properly managed.
746-751: Improved node removal success conditionThe function now returns success if any type of node metadata was successfully removed, making it more flexible.
758-760: Enhanced function documentationImproved the description of
remove_node_metadatato clarify its purpose and return value.
768-769: Updated metadata filtering by node typeModified the file filtering logic to use the node type for consistent filtering across the codebase.
797-801: Improved function documentationEnhanced the documentation for
fetch_node_metricsto better describe its purpose and behavior.
884-896: Extended cluster metrics to include querier nodesUpdated the cluster metrics gathering to concurrently fetch metrics from all node types, providing a complete view of the cluster.
897-902: Added querier metrics handlingThe function now properly processes querier metrics alongside ingestor and indexer metrics, maintaining a consistent approach across all node types.
Also applies to: 913-928
src/parseable/mod.rs (6)
50-51: Updated import to use the global metadata singletonImporting
INGESTOR_METAfromingest_serverto access the globally cached ingestor metadata.
165-168: Replaced direct metadata access with singleton approachUpdated to use the global
INGESTOR_METAsingleton instead of a struct field, aligning with the new metadata management approach.
182-190: Simplified stream checking logicThe function now returns immediately if the stream exists in memory, and only attempts to load from storage for Query or Prism mode, making the logic clearer.
254-255: Added support for Prism modeUpdated the server mode string function to include the new Prism mode.
312-315: Updated to use global metadata singletonReplaced direct metadata access with the global
INGESTOR_METAsingleton when creating streams from storage.
461-465: Extended stream existence check to include Prism modeAdded support for the Prism mode when checking if a stream exists in storage, enabling proper functionality in this new operational mode.
src/handlers/http/modal/mod.rs (6)
19-61: Confirm Default Version AlignmentThe introduction of
DEFAULT_VERSIONas"v4"looks consistent with the v3-to-v4 migration. However, please verify that all consumers relying on metadata version checks can handle"v4". If there are older references or partial migrations elsewhere, consider adding explicit checks or backward-compatibility logic.
202-240: Enum Expansion Looks GoodThe new
NodeTypeenum (including thePrismvariant) and its default ofIngestoraligns well with the broader refactor. Ensure that defaulting toIngestoris fully intentional, especially if multiple node types are expected to be configured in certain deployments.
280-333: Validate Load Logic for Multiple Matches
load_node_metadatacurrently returns the first stored metadata matching the current domain name and port. If multiple metadata files exist, only the first match is processed. Confirm if this is the intended behavior, especially in clusters with multiple nodes referencing similar endpoints.
414-443: Logging Configuration UpdatesThe
update_metadatafunction properly logs updates to domain name, port, and credentials, facilitating easier debugging. This approach is straightforward and consistent with best practices around configuration changes.
487-532: Legacy Migration HandlingThe migration code transforms v3 metadata into v4 by renaming
ingestor_idorindexer_idtonode_id. If older Prism or Querier IDs never existed, this is fine. Otherwise, consider adding similar migration logic for them.
553-580: Type Aliases for Backward CompatibilityUsing type aliases (
IngestorMetadata,IndexerMetadata, etc.) preserves backward compatibility without duplicating struct definitions. This is a pragmatic strategy to ease the transition to the unifiedNodeMetadata.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/handlers/http/cluster/mod.rs (1)
727-766:⚠️ Potential issuePrism node metadata is never removed, causing a potential leftover entry.
When removing a node, only ingestor, indexer, and querier metadata are scheduled for deletion, ignoringPrismMetadata.// Delete ingestor metadata let removed_ingestor = remove_node_metadata::<IngestorMetadata>(&object_store, &domain_name, NodeType::Ingestor).await?; // Delete indexer metadata let removed_indexer = remove_node_metadata::<IndexerMetadata>(&object_store, &domain_name, NodeType::Indexer).await?; // Delete querier metadata let removed_querier = remove_node_metadata::<QuerierMetadata>(&object_store, &domain_name, NodeType::Querier).await?; +// Delete prism metadata +let removed_prism = + remove_node_metadata::<NodeMetadata>(&object_store, &domain_name, NodeType::Prism).await?; if removed_ingestor || removed_indexer || removed_querier + || removed_prism { return Ok(( format!("node {} removed successfully", domain_name), StatusCode::OK, )); }
🧹 Nitpick comments (3)
src/handlers/http/cluster/mod.rs (3)
550-550: Synchronize the doc comment with the actual node types.
Currently, it mentions ingestor, indexer, and querier, but the code also fetches Prism node info.-/// Fetches cluster information for all nodes (ingestor, indexer, and querier) +/// Fetches cluster information for all node types (ingestor, indexer, querier, and prism)
708-710: Doc comment omits Prism usage.
The lines above mention only ingestor, indexer, and querier, but this function also supports prism.-/// this is used to get the node info for ingestor, indexer and querier +/// this is used to get the node info for ingestor, indexer, querier, and prism
795-795: Possible issue if multiple metadata files match the same domain name.
Right now, only the first item is used, and the rest (if any) remain. Consider a loop if duplicates are possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs(14 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (31)
src/handlers/http/cluster/mod.rs (31)
57-59: No concerns with these import statements.
They appear necessary given the subsequent usage of these types.
78-82: The error handling for retrieving ingestor info looks consistent.
No issues found with this pattern.
128-131: Same ingestor retrieval pattern here; no concerns.
179-182: Same ingestor retrieval pattern here; no concerns.
227-230: Same ingestor retrieval pattern here; no concerns.
283-289: Same ingestor retrieval pattern here; no concerns.
335-338: Same ingestor retrieval pattern here; no concerns.
553-560: Concurrent fetching logic for all four node types looks fine.
562-567: Error-mapping pattern for prism_result is consistent with other nodes.
570-575: Error-mapping pattern for querier_result is consistent.
578-578: Ingestor metadata retrieval pattern here is consistent.
586-586: Indexer metadata retrieval is handled similarly. No concerns.
594-599: Concurrent fetching of node info via future::join4 is well-structured.
603-607: Combining node info vectors is straightforward; no issues found.
611-613: Doc comment here accurately reflects the subsequent single-node fetch flow.
713-715: Capturing the prefix in a closure is a clean approach for filtering.
718-719: Using a closure to filter metadata by node type is clear and concise.
769-775: Documentation for the remove_node_metadata function is consistent with its behavior.
779-779: Filtering by node type prefix for deletes is consistent with get_node_info.
808-812: Doc comments accurately describe the node-metrics-fetch flow.
873-875: Early return on an empty node list is a clean short-circuit.
895-899: High-level doc comment is consistent with the function's multi-node metrics approach.
901-906: Using future::join4 for concurrency to gather node info remains consistent here.
907-908: No issues with awaiting the concurrency future.
909-913: Prism metadata error handling is consistent with the pattern across the codebase.
915-919: Querier metadata error handling remains consistent.
921-921: Ingestor metadata error handling is aligned with prior occurrences.
926-926: Indexer metadata retrieval code remains uniform in error handling.
931-937: Fetching node metrics concurrently with future::join4 is an effective approach.
942-947: Prism metrics merging is consistent with other node types.
948-953: Querier metrics merging is similarly consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/handlers/http/cluster/mod.rs (6)
128-131: Repeated pattern for fetching ingestor info.
178-182: Repeated pattern for fetching ingestor info.
227-230: Repeated pattern for fetching ingestor info.
285-289: Repeated pattern for fetching ingestor info.
334-338: Repeated pattern for fetching ingestor info.
722-723: Avoid silently ignoring parse errors (previously noted).
These lines still discard any JSON deserialization errors via.ok(), repeating the concern from past reviews. Consider logging the parse failures instead of ignoring them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs(14 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/handlers/http/cluster/mod.rs (2)
src/handlers/http/modal/mod.rs (5)
new(256-278)node_type(553-553)node_type(566-568)domain_name(551-551)domain_name(558-560)src/handlers/http/cluster/utils.rs (4)
new(38-50)new(65-83)new(98-116)new(128-135)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka aarch64-apple-darwin
- 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 Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (31)
src/handlers/http/cluster/mod.rs (31)
57-59: No issues with the new imports.
These imports align with the new unified node metadata approach.
550-550: Good documentation added.
This doc comment clarifies that all node types are included.
552-575: Parallel fetching for all node types looks solid.
Usingfuture::join4to fetch metadata for prism, querier, ingestor, and indexer concurrently is a clean approach. Logging errors per-node-type helps diagnose failures quickly.
578-578: Fetching ingestor metadata snippet.
Looks consistent with the concurrency pattern above.
586-586: Fetching indexer metadata snippet.
Identical pattern for retrieving indexer metadata—no issues.
593-596: Good concurrency structure for fetching node info.
Parallelizing these calls reduces overall wait time for cluster information retrieval.
604-605: Aggregating prism and querier info.
Straightforward extension of the combined results vector.
611-613: Helpful doc comments.
Explains the single-node fetch flow clearly.
705-708: Excellent doc comment to introduce get_node_info.
The function purpose is clear for each node type.
709-710: Function signature is flexible and generic.
TyingMetadata + DeserializeOwnedto aNodeTypeparameter is a clean design.
713-713: Capturing the node type prefix.
Filtering bynode_type.to_string()is concise and consistent with the rest of the code.
718-718: File prefix closure.
Uses the stored string prefix to filter metadata files effectively.
725-725: Returning the collected metadata.
Looks good.
727-731: Clear documentation on node removal logic.
These doc comments help new contributors follow the removal flow.
744-745: Removing ingestor metadata.
Matches the new unified approach for node removal.
749-750: Removing indexer metadata.
Consistent with the new node removal logic.
752-756: Removing querier metadata.
Parallels the ingestor and indexer removal flow.
757-760: Removing prism metadata.
Adds prism support to the node removal function.
761-770: Return states after node metadata removal.
Handles success for any node type and returns a not found error otherwise.
773-774: Helpful doc comment for remove_node_metadata.
Explains expected return value clearly.
778-778: New NodeType parameter.
Ensures each node type is handled properly.
783-783: Filtering by node type prefix.
Follows the standard approach used in get_node_info.
812-815: Doc comments for fetch_node_metrics.
Clarify the single-node metric retrieval process.
877-879: Short-circuit on empty node list.
Prevents unnecessary metric fetch operations when no nodes exist.
905-917: fetch_cluster_metrics concurrency.
Collects prism, querier, ingestor, and indexer metadata in parallel. Reasonable error handling.
919-923: Querier metadata retrieval.
Consistently logs errors and returns early if metadata fetching fails.
925-925: Ingestor metadata retrieval in cluster metrics.
Follows the same parallel pattern.
930-930: Indexer metadata retrieval in cluster metrics.
Again, consistent usage of parallel pattern.
935-937: Parallel metric fetching invocation.
Usingfuture::join4to retrieve node metrics is well-structured.
946-950: Extending the final metrics with prism metrics.
Logs an error if any fetch fails, ensuring issues are visible.
952-956: Extending the final metrics with querier metrics.
Same flow, continuing consistent error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/cluster/mod.rs (1)
711-732: Avoid silently ignoring parse errors.This code uses
filter_map(|x| serde_json::from_slice::<T>(x).ok()), which discards deserialization failures without logging. This repeats a past review comment. Consider logging the error to aid debugging.
🧹 Nitpick comments (1)
src/handlers/http/cluster/mod.rs (1)
68-91: Consider optional concurrent processing of ingestors.Currently,
for_each_live_ingestorprocesses each ingestor sequentially and fails on the first encountered error. If partial success is acceptable, or you need more throughput, consider a parallel approach (e.g., collecting tasks in ajoin_all). Otherwise, this is fine for smaller clusters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default 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
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (18)
src/handlers/http/cluster/mod.rs (18)
23-23: Good addition ofFutureimport.Bringing
std::future::Futureexplicitly into scope clarifies the intent for async function parameters.
58-60: Clean import grouping of metadata types.Centralizing the various node metadata imports under
modalhelps maintain clarity about node-type objects in use.
105-145: Synchronized stream updates look good.This block correctly clones headers and body per ingestor and handles network errors gracefully. If partial stream update failures matter, consider collecting and reporting each ingestor’s outcome instead of logging only to error.
150-198: Role update synchronization is handled consistently.Uses the same
for_each_live_ingestorpattern, which is consistent across the codebase. The error logging is adequate for diagnosing failures.
201-238: User deletion synchronization behaves similarly.This follows the same approach—skips offline ingestors, pushes a delete request to each live ingestor, and logs any errors. This is acceptable for basic synchronization.
252-297: User creation request distribution.Again, the approach is consistent. Serialization errors are handled, and offline ingestors are skipped. This uniform pattern across functions improves maintainability.
300-338: Password reset propagation is also consistent.Matches the established pattern of distributing requests to all live ingestors. Good consistency in logging and error handling.
345-383: Shared role updates.Likewise, this function correctly forwards updates and logs errors. The overall design is coherent.
556-582: Parallel retrieval of node metadata is well-structured.Using
future::join4for prism, querier, ingestor, and indexer metadata retrieval is efficient and consistent. Error handling withmap_erris clear.
599-614: Concurrent node info fetching.Retrieving node info is done in parallel with
join4, which is a solid approach. The finalinfosvector merges results neatly.
618-619: Helpful doc comments forfetch_node_info.These clarifications make it easier to understand the purpose and flow of the function.
733-777: Removing multiple node types is well-integrated.Successfully accommodates prism, querier, ingestor, and indexer. Checks for liveness before removal.
779-816: Genericremove_node_metadatais clear.Filtering files by node type prefix is straightforward for the current architecture. Good usage of typed metadata.
818-822: Doc comments forfetch_node_metrics.These clarifications highlight the function’s purpose and expected behavior.
883-885: Early return for empty node list.Returning an empty metrics list when no relevant nodes exist avoids unnecessary processing.
905-909: Comprehensive doc comments for cluster metrics fetch.These well-organized notes outline the overall flow.
911-930: Parallel fetching and error bundling for node metrics.Again,
join4concurrency is used, matching the earlier approach. Error handling is consistent via mapping intoPostError.
952-963: Prism and querier metrics integrated seamlessly.Combining metrics from different node types unifies cluster metrics gathering, which is beneficial for consistent observability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/cluster/mod.rs (1)
740-740: Avoid silently ignoring parse errors.Currently,
serde_json::from_slice::<T>(x).ok()discards any errors without logging or handling. This may mask data issues.- .filter_map(|x| serde_json::from_slice::<T>(x).ok()) + .filter_map(|x| { + match serde_json::from_slice::<T>(x) { + Ok(val) => Some(val), + Err(e) => { + error!("Failed to parse node metadata: {:?}", e); + None + } + } + })
🧹 Nitpick comments (1)
src/handlers/http/cluster/mod.rs (1)
775-777: Consider using a specific type for prism metadata.While other node types use specific metadata types like
IngestorMetadata,IndexerMetadata, andQuerierMetadata, prism nodes are using the genericNodeMetadata. For consistency, consider creating a dedicatedPrismMetadatatype.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (5)
src/handlers/http/cluster/mod.rs (5)
68-102: Great abstraction with the newfor_each_live_ingestorfunction!This is an excellent refactoring that eliminates duplicate code patterns across multiple synchronization functions. The generic function properly handles error propagation, checks node liveness, and processes operations in parallel.
723-743: Well-designed generic function for node info retrieval.The generic
get_node_info<T>function elegantly replaces specific functions for each node type, improving code reusability and consistency. It properly filters object store keys by node type prefix and deserializes into the appropriate type.
567-626: Great enhancement to include all node types in cluster info.The updated
get_cluster_infofunction now properly handles prism and querier nodes alongside ingestor and indexer nodes. The concurrent fetching of node metadata and info is efficient and well-structured.
894-896: Good optimization with early return for empty node lists.Adding an early return for empty node lists avoids unnecessary processing and improves function efficiency.
916-988: Comprehensive refactoring to include all node types in metrics collection.The updated
fetch_cluster_metricsfunction now properly collects metrics from all node types (ingestor, indexer, querier, and prism) in parallel and combines them into a single vector. This is a well-structured enhancement.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Nikhil Sinha <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/handlers/http/cluster/mod.rs (1)
725-751: Excellent generic function for node info retrieval.The new generic
get_node_info<T>function unifies the previously separate functions for fetching different node types' metadata. This improves code reuse and maintainability.The error handling has been improved to log parsing errors instead of silently ignoring them, addressing a previous review comment.
🧹 Nitpick comments (3)
src/handlers/http/cluster/mod.rs (3)
68-102: Well-structured abstraction for ingestor operations.This new generic helper function
for_each_live_ingestoreffectively abstracts the common pattern of fetching ingestor info, checking liveness, and executing a function on each live ingestor. The function:
- Uses proper generic bounds
- Handles concurrency appropriately with
futures::future::join_all- Includes good error handling and propagation
- Improves code maintainability by removing duplication
Consider making this function even more generic by renaming it to
for_each_live_nodeand accepting aNodeTypeparameter, similar toget_node_info. This would allow it to be used for any node type, not just ingestors.-pub async fn for_each_live_ingestor<F, Fut, E>(api_fn: F) -> Result<(), E> +pub async fn for_each_live_node<F, Fut, E>(node_type: NodeType, api_fn: F) -> Result<(), E>And then update the implementation accordingly:
- let ingestor_infos: Vec<NodeMetadata> = - get_node_info(NodeType::Ingestor).await.map_err(|err| { - error!("Fatal: failed to get ingestor info: {:?}", err); + let node_infos: Vec<NodeMetadata> = + get_node_info(node_type).await.map_err(|err| { + error!("Fatal: failed to get {} info: {:?}", node_type, err);
567-626: Excellent extension of cluster info to include all node types.The
get_cluster_infofunction has been expanded to concurrently fetch metadata and detailed info for all node types (prism, querier, ingestor, indexer). This is a good improvement for completeness and performance.The concurrent fetching with
future::join4is well implemented, with proper error handling for each node type.Consider further reducing duplication in error handling for each node type, perhaps with a helper function.
928-996: Excellent extension of metrics collection to all node types.The
fetch_cluster_metricsfunction has been extended to concurrently fetch metrics from all node types (prism, querier, ingestor, indexer) and combine them into a single result. This is a good improvement for completeness and system monitoring capabilities.Consider refactoring the repeated pattern of error handling for each node type's metrics to reduce code duplication, perhaps with a helper function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/handlers/http/cluster/mod.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
🔇 Additional comments (11)
src/handlers/http/cluster/mod.rs (11)
58-60: Good update to imports with unified node type system.The imports have been updated to include the new unified
NodeMetadataandNodeTypesystem along with specific metadata types for different node types. This supports the refactoring of node handling throughout the file.
120-157: Good refactoring of stream synchronization.The
sync_streams_with_ingestorsfunction has been refactored to use the newfor_each_live_ingestorhelper, which simplifies the implementation and reduces code duplication. The error handling and request construction are well-implemented.
171-208: Good refactoring of role synchronization.The
sync_users_with_roles_with_ingestorsfunction has been nicely refactored to use the newfor_each_live_ingestorhelper, improving code clarity and reducing duplication.
212-249: Good refactoring of user deletion synchronization.The
sync_user_deletion_with_ingestorsfunction has been refactored to use the newfor_each_live_ingestorhelper, simplifying the implementation and improving maintainability.
270-308: Good refactoring of user creation synchronization.The
sync_user_creation_with_ingestorsfunction has been refactored to use the newfor_each_live_ingestorhelper, making the code more consistent and easier to maintain.
314-349: Good refactoring of password reset synchronization.The
sync_password_reset_with_ingestorsfunction has been refactored to use the newfor_each_live_ingestorhelper, which simplifies the implementation and brings it in line with other similar functions.
356-394: Good refactoring of role update synchronization.The
sync_role_update_with_ingestorsfunction has been refactored to use the newfor_each_live_ingestorhelper, improving code consistency and reducing duplication.
628-631: Improved documentation for fetch_node_info.The function comment has been expanded to clarify that it calls the node's
/aboutendpoint and constructs aClusterInfostruct, which improves code documentation.
757-796: Good extension of node removal to handle all node types.The
remove_nodefunction has been updated to attempt removal of metadata for all node types (prism, querier, ingestor, indexer), making it more comprehensive.
800-809: Well-designed generic node metadata removal.The
remove_node_metadatafunction has been updated to take aNodeTypeparameter, making it more generic and reusable across different node types.
902-904: Good optimization for empty node lists.Adding an early return for empty node lists in
fetch_nodes_metricsis a nice optimization that avoids unnecessary processing.
Summary by CodeRabbit
New Features
Enhancements