-
Notifications
You must be signed in to change notification settings - Fork 384
fix(metric-engine): properly propagate errors during batch open operation #6325
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
Conversation
…tion Signed-off-by: WenyXu <[email protected]>
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.
Pull Request Overview
This PR fixes error propagation issues in the metric engine’s batch open operation by adjusting error handling and consolidating state recovery.
- Introduces a new error variant (NoOpenRegionResult) for missing open region results.
- Refactors error propagation and state recovery flows in the engine’s sync, open, and catchup modules.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/metric-engine/src/error.rs | Adds new error variant and updates error status mapping. |
src/metric-engine/src/engine/sync.rs | Removes redundant primary key encoding retrieval in state recovery. |
src/metric-engine/src/engine/open.rs | Refactors open logic; converts physical region IDs to a HashMap and adjusts error propagation. |
src/metric-engine/src/engine/catchup.rs | Updates state recovery to align with new error propagation design. |
Comments suppressed due to low confidence (2)
src/metric-engine/src/engine/open.rs:103
- [nitpick] Consider logging additional context or including a comment explaining the error wrapping with BoxedError to help future debugging of failure cases in open_physical_region_with_results.
let _ = metadata_region_result.context(NoOpenRegionResultSnafu {
src/metric-engine/src/engine/catchup.rs:82
- [nitpick] Since the primary key encoding is now retrieved internally within recover_states, please add an inline comment or update the function’s docstring to document this design change for clarity.
self.recover_states(region_id, physical_region_options)
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.
LGTM
Signed-off-by: WenyXu <[email protected]>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This PR fixes error propagation issues in the metric engine's batch open operation. Previously, underlying errors during batch open operation were not properly propagated to the caller, which could lead to bad user experience.
After:
Before:
PR Checklist
Please convert it to a draft if some of the following conditions are not met.