bugfixes #1395
bugfixes #1395
Conversation
WalkthroughThis change refactors alert state management by introducing an asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant HTTP_Handler
participant AlertManager
participant Alert (Trait Object)
participant Storage
HTTP_Handler->>AlertManager: get_alert_by_id(alert_id)
AlertManager->>Alert: (returns Box<dyn AlertTrait>)
HTTP_Handler->>Alert: update_state(is_manual, new_state, trigger_notif)
Alert->>Alert: Validate state transition
Alert->>Storage: Persist updated state (async)
Alert->>Alert: Trigger notification (if applicable)
Alert-->>HTTP_Handler: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
src/alerts/mod.rs (1)
1523-1523: Remove commented code.The commented
storevariable should be removed since storage operations are now handled within the alert'supdate_statemethod.- // let store = PARSEABLE.storage.get_object_store(); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/alerts/alert_types.rs(2 hunks)src/alerts/alerts_utils.rs(1 hunks)src/alerts/mod.rs(2 hunks)src/alerts/traits.rs(2 hunks)src/handlers/http/alerts.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
src/alerts/alerts_utils.rs (1)
Learnt from: nikhilsinhaparseable
PR: #1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
src/handlers/http/alerts.rs (2)
Learnt from: nikhilsinhaparseable
PR: #1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
Learnt from: nikhilsinhaparseable
PR: #1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
src/alerts/alert_types.rs (1)
Learnt from: nikhilsinhaparseable
PR: #1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
src/alerts/traits.rs (1)
Learnt from: nikhilsinhaparseable
PR: #1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
src/alerts/mod.rs (1)
Learnt from: nikhilsinhaparseable
PR: #1388
File: src/alerts/mod.rs:88-104
Timestamp: 2025-07-24T11:09:21.781Z
Learning: In the Parseable alert system (src/alerts/mod.rs), alert versions are server-generated and controlled via CURRENT_ALERTS_VERSION constant, not user input. The AlertVerison enum's From<&str> implementation correctly defaults unknown versions to V2 since the server only generates known versions (v1, v2). Unknown versions would only occur in exceptional cases like file corruption, making the current fallback approach appropriate.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-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
🔇 Additional comments (7)
src/alerts/alerts_utils.rs (1)
140-141: Good refactoring for clarity.Separating the array check from the empty check improves code readability and makes the validation logic more explicit.
src/handlers/http/alerts.rs (2)
135-137: LGTM! Proper trait abstraction.The changes correctly use the trait getter method and return the configuration representation, maintaining API compatibility while working with the new trait-based architecture.
194-220: Ignore the alert reinsertion optimization suggestion
The clone → mutate → reinsert pattern is intentional in our trait-based design.
get_alert_by_idreturns a freshBox<dyn AlertTrait>(viaclone_box)- You then call
alert.update_state(…)on that clone- Finally,
alerts.update(&*alert)reinserts it, invokingclone_box()againAvoiding these clones would require a major API change (e.g. storing mutable trait objects or changing method signatures to
&mut self), which is out of scope. No action required.Likely an incorrect or invalid review comment.
src/alerts/alert_types.rs (2)
214-237: State transition rules are well-defined.The validation logic correctly enforces state transition rules for manual updates. The restrictions prevent invalid state transitions and maintain alert lifecycle integrity.
241-241:put_alertsignature exists inObjectStorage
I’ve confirmed that theasync fn put_alert(&self, alert_id: Ulid, alert: &AlertConfig)method is declared in theObjectStoragetrait (see src/storage/object_storage.rs). No further changes are needed.src/alerts/traits.rs (1)
33-38: Clean trait evolution for async state management.The addition of the async
update_statemethod and the change to return trait objects fromget_alert_by_idproperly support the new architecture. The method signature provides all necessary parameters for state validation, persistence, and notification handling.Also applies to: 65-65
src/alerts/mod.rs (1)
1527-1549: Write-lock ensures atomic updates – no race hereThe
self.alerts.write().awaitguard serializes all mutations, so reading the entry, updating its state, and reinserting it happen under the same write lock. There is no interleaving that could leave the map in an inconsistent state.If you later spot contention on this lock (e.g., slow
update_statecalls blocking readers), you can refactor theAlertTraitimplementation to hold its mutable fields behind a finer-grained mutex or atomic, allowing in-place state changes without locking the entire alerts map.Likely an incorrect or invalid review comment.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes