-
-
Notifications
You must be signed in to change notification settings - Fork 137
optimise server start #1277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimise server start #1277
Conversation
parallelise stream migration load calls - correlations, filters, dashboards, alerts in parallel remove file migration - deprecate for migrations < 1.0.0
WalkthroughThis PR enhances error handling and control flow across several modules. A new Changes
Sequence Diagram(s)sequenceDiagram
participant S as Initialization Caller
participant L as load_on_init
participant C as CORRELATIONS
participant F as FILTERS
participant D as DASHBOARDS
participant A as ALERTS
S->>L: Call load_on_init()
L->>C: Load CORRELATIONS
L->>F: Load FILTERS
L->>D: Load DASHBOARDS
L->>A: Load ALERTS
C-->>L: Return result
F-->>L: Return result
D-->>L: Return result
A-->>L: Return result
L-->>S: Return overall result
sequenceDiagram
participant M as run_migration
participant Streams as Stream Migrations
M->>Streams: Dispatch concurrent migration tasks (using join_all)
Streams-->>M: Return individual statuses/warnings
M->>M: Aggregate results and report errors if any
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/migration/mod.rs (1)
134-179
: Consider bounding concurrency and returning detailed errors.While running stream migrations in parallel with
join_all
is efficient, be mindful of large-scale scenarios where unbounded concurrency can overwhelm system resources. Also consider returning or logging a more detailed error list for troubleshooting, rather than aggregating the total count of failures into a single message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/alerts/mod.rs
(4 hunks)src/handlers/http/cluster/mod.rs
(1 hunks)src/handlers/http/modal/mod.rs
(3 hunks)src/handlers/http/modal/query_server.rs
(3 hunks)src/handlers/http/modal/server.rs
(3 hunks)src/migration/mod.rs
(3 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
src/handlers/http/cluster/mod.rs (1)
src/handlers/http/cluster/utils.rs (1)
check_liveness
(172-192)
src/handlers/http/modal/mod.rs (3)
src/hottier.rs (1)
futures
(755-757)src/parseable/mod.rs (1)
storage
(235-237)src/rbac/map.rs (1)
users
(39-45)
src/handlers/http/modal/server.rs (1)
src/handlers/http/modal/mod.rs (1)
load_on_init
(167-200)
src/alerts/mod.rs (4)
src/handlers/http/query.rs (1)
from
(346-348)src/users/dashboards.rs (1)
load
(113-178)src/users/filters.rs (1)
load
(76-131)src/correlation.rs (1)
load
(55-75)
⏰ 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 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
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (12)
src/handlers/http/cluster/mod.rs (1)
723-727
: Great addition of liveness check before fetching metrics.This change improves error handling by preventing unnecessary network requests to unavailable ingestors, which can enhance performance and provide clearer logs about the system state.
src/alerts/mod.rs (4)
708-709
: Good addition of Anyhow variant to standardize error handling.Adding the
Anyhow
variant toAlertError
enables better integration with other components that use anyhow for error handling, supporting the migration to a more consistent error handling approach across the codebase.
724-725
: Consistent error handling for the new Anyhow variant.Good implementation of the status code handling for the new
Anyhow
variant, maintaining consistency with other error variants by returning an appropriateINTERNAL_SERVER_ERROR
status code.
737-737
: Updated return type to use anyhow::Result.Changing the return type to
anyhow::Result<()>
aligns with the broader initiative to standardize error handling across the codebase using the anyhow crate.
750-751
: Improved error mapping with anyhow.Using
map_err
to convert errors toanyhow::Error
with a meaningful message improves error context and makes debugging easier.src/handlers/http/modal/mod.rs (1)
167-200
: Excellent implementation of parallel loading to optimize startup.The new
load_on_init
function implements concurrent loading of multiple components usingfuture::join4
, which should significantly improve server startup time. Key strengths:
- Each component (correlations, filters, dashboards, alerts) loads in parallel
- Error handling is robust - failures in one component won't stop others from loading
- All errors are properly logged without halting execution
- The function leverages context from anyhow to provide clearer error messages
This implementation directly supports the PR objective of optimizing server startup by parallelizing component loading.
src/handlers/http/modal/server.rs (2)
102-107
: Removed deprecated file migration support as intended.The commented code and TODO note clearly indicate the removal of file migration support for deployments less than version 1.0.0, which aligns with the PR objectives.
121-121
: Simplified initialization with concurrent loading.Replaced individual component loading with a single call to
load_on_init()
, leveraging the new concurrent loading mechanism. This change streamlines the code while improving performance through parallelization.src/migration/mod.rs (1)
24-31
: Imports look fine and align with new usage.No issues found here.
Also applies to: 40-40
src/handlers/http/modal/query_server.rs (3)
36-36
: Updated imports are consistent with the revised initialization flow.No concerns with these added imports.
Also applies to: 42-42
107-108
: Consolidate initialization logic.Invoking
load_on_init()
centralizes loading tasks, enhancing maintainability. No issues found.
134-139
: Returning the result properly ensures reliable error propagation.Switching to
Ok(result)
helps maintain consistency in your async error handling.
parallelise stream migration
load calls - correlations, filters, dashboards, alerts in parallel
remove file migration - deprecate for migrations < 1.0.0
Summary by CodeRabbit
New Features
Bug Fixes
Refactor