Handle watchers with future sequence numbers gracefully #4773
Conversation
WalkthroughThe changes focus on enhancing the event iterator logic in the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)pkg/lib/watcher/watcher_test.go (3)
The test case correctly verifies that the watcher continues from the specified sequence number when no checkpoint exists.
The test cases thoroughly verify the behavior of different iterator types when the store is empty, ensuring consistent handling by starting from sequence 0.
These test cases are crucial for ensuring the watcher gracefully handles sequence numbers that are at or beyond the latest event. They verify that:
This aligns perfectly with the PR objective of handling watchers with future sequence numbers gracefully. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (2)
pkg/lib/watcher/watcher_test.go (1)
116-116: Correct the mismatched sequence number in the commentThe comment indicates storing events up to sequence number 15, but
setupLatestEventis set toptr(uint64(10)). Please update the comment or the code to reflect the actual behavior.pkg/lib/watcher/watcher.go (1)
111-117: Clarify log message for sequence number adjustmentWhen the requested sequence number exceeds the latest, the log message states: "requested sequence number is higher than latest, starting from latest instead". To enhance clarity, consider specifying that the watcher will start after the latest sequence number, effectively waiting for new events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/lib/watcher/watcher.go(1 hunks)pkg/lib/watcher/watcher_test.go(3 hunks)
🔇 Additional comments (3)
pkg/lib/watcher/watcher_test.go (2)
185-189: Ensure consistency in test case expectations
In the test case "Sequence too high, start after latest", the initialIter is set to watcher.AtSequenceNumberIterator(20) while the setupLatestEvent is ptr(uint64(15)). The expectedIter is watcher.AfterSequenceNumberIterator(15). Verify that this is the intended behavior and that the expectedIter aligns with the logic in determineStartingIterator.
191-195: Ensure correctness in "After sequence too high" test case
Similarly, in the test case "After sequence too high, start after latest", confirm that setting expectedIter to watcher.AfterSequenceNumberIterator(15) when initialIter is watcher.AfterSequenceNumberIterator(20) and setupLatestEvent is ptr(uint64(15)) is the intended behavior.
pkg/lib/watcher/watcher.go (1)
95-118: Improved handling of initial event iterators in determineStartingIterator
The updated logic correctly handles the EventIteratorTrimHorizon by returning it directly, and adjusts the starting iterator appropriately for EventIteratorLatest and when the requested sequence number exceeds the latest. This ensures the watcher starts from the correct position according to the iterator type.
wdbaruni
deleted the
eng-453-handle-watchers-with-future-sequence-numbers-gracefully
branch
Summary by CodeRabbit
New Features
Bug Fixes
Tests