eventstore,eventservice: propagate event iterator errors#4985
eventstore,eventservice: propagate event iterator errors#4985
Conversation
📝 WalkthroughWalkthroughThe PR propagates event store iterator errors through the entire scanning pipeline. ChangesEvent Store Iterator Error Propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
lidezhu
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the EventStore and EventScanner to properly handle and propagate errors during iterator creation and closure. Key changes include modifying the GetIterator signature to return an error, implementing more robust error checking when initializing Pebble iterators, and updating the eventScanner to handle errors from both scanning and closing iterators. Feedback focuses on potential resource leaks due to the removal of defer in the scanner and a suggestion to simplify the error handling logic when checking the first element of a Pebble iterator.
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
# Conflicts: # logservice/eventstore/event_store.go
ti-chi-bot
Bot
added
size/L
|
/gemini summary |
Summary of ChangesThis pull request significantly improves the error handling and propagation mechanisms within the event store and event service components. By modifying iterator interfaces and implementations to return errors, the system can now reliably detect and react to failures during event iteration, preventing potential resource leaks and enhancing overall stability. The changes ensure that errors are not silently ignored but are instead properly surfaced and managed throughout the event processing pipeline. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/eventservice/event_scanner_test.go (1)
61-96: 💤 Low valueLGTM – both error propagation paths are correctly covered.
Minor coverage gap: the
scanErr != nil && closeErr != nilbranch (line 148–152 inevent_scanner.go, which logs a warning and returnsscanErr) is not exercised. Worth adding a test case for completeness, though it is low-risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/eventservice/event_scanner_test.go` around lines 61 - 96, Add a test case in TestEventScannerReturnsIteratorErrors that constructs a scanner using newEventScanner with a stubEventGetter whose iterator returns a non-nil error from Next (scanErr) and also returns a non-nil error from Close (closeErr) (e.g. &mockEventIterator{nextErr: scanErr, closeErr: closeErr}), call scanner.scan(...) with the same disp/dataRange, and assert the returned error is scanErr, events is nil and interrupted is false to exercise the scanErr != nil && closeErr != nil branch in event_scanner.scan; reuse NewMockSchemaStore and mockMounter as in the other cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/eventservice/event_scanner_test.go`:
- Around line 61-96: Add a test case in TestEventScannerReturnsIteratorErrors
that constructs a scanner using newEventScanner with a stubEventGetter whose
iterator returns a non-nil error from Next (scanErr) and also returns a non-nil
error from Close (closeErr) (e.g. &mockEventIterator{nextErr: scanErr, closeErr:
closeErr}), call scanner.scan(...) with the same disp/dataRange, and assert the
returned error is scanErr, events is nil and interrupted is false to exercise
the scanErr != nil && closeErr != nil branch in event_scanner.scan; reuse
NewMockSchemaStore and mockMounter as in the other cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 417bb8d7-f9c3-4f73-ac38-b12d2338e81d
📒 Files selected for processing (6)
logservice/eventstore/event_store.gologservice/eventstore/event_store_test.gopkg/eventservice/event_broker.gopkg/eventservice/event_scanner.gopkg/eventservice/event_scanner_test.gopkg/eventservice/event_service_test.go
💤 Files with no reviewable changes (1)
- pkg/eventservice/event_broker.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
2 participants
What problem does this PR solve?
Issue Number: close #5005
What is changed and how it works?
This pull request significantly improves the error handling and propagation mechanisms within the event store and event service components. By modifying iterator interfaces and implementations to return errors, the system can now reliably detect and react to failures during event iteration, preventing potential resource leaks and enhancing overall stability. The changes ensure that errors are not silently ignored but are instead properly surfaced and managed throughout the event processing pipeline.
Highlights
GetIteratormethod inEventStoreand its corresponding interface to return errors, ensuring that issues during iterator creation or retrieval are properly surfaced.eventStore.GetIteratorforpebble.NewIteranditer.First()calls, preventing silent failures and ensuring resource cleanup.EventIterator.Closemethod to return any accumulated iterator errors, providing a comprehensive view of potential issues during iteration lifecycle.eventScannerto correctly handle and propagate errors returned byGetIteratorandClose, improving the reliability of event processing.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Release Notes