Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical logic bug in the bufferedMap subscription where the watermark optimization check was inverted. The bug caused keys below the low watermark (already copied by the copier) to be incorrectly skipped during flush operations, while keys at or above the watermark (not yet copied) were incorrectly flushed. This could lead to data race conditions, contention, and performance/memory issues from buffering too many versions.
Changes:
- Fixed the watermark check condition in
bufferedMap.Flush()by adding a negation operator to match deltaMap behavior - Updated test expectations in
TestBufferedMapFlushWithoutLockRespectsWatermarkto verify the correct behavior - Updated comments in the test to accurately describe the fixed watermark logic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/repl/subscription_buffered.go | Fixed inverted watermark logic by changing condition from KeyBelowLowWatermark(...) to !KeyBelowLowWatermark(...) and updated debug log message |
| pkg/repl/subscription_buffered_test.go | Updated test expectations and comments to reflect the corrected watermark behavior, verifying that keys below the watermark are now flushed and keys at/above are skipped |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Kiran01bm
left a comment
There was a problem hiding this comment.
lgtm - also good that normal migrations (without buffering) was not impacted by this bug.
A Pull Request should be associated with an Issue.
Fixes #624