[fix](editlog) Fix JournalObservable.upperBound() off-by-one in binary search#62065
[fix](editlog) Fix JournalObservable.upperBound() off-by-one in binary search#62065dataroaring wants to merge 1 commit intoapache:masterfrom
Conversation
…y search The binary search uses right = size - 1 and right = middle - 1, which is a lower_bound variant that can miss notifying observers. Specifically, when size == 1 and the single observer's targetJournalVersion <= journalId, the function sets right = size - 1 = 0, then the post-loop check compares value >= rightValue and returns right + 1 = 1. But when size > 1 and the matching element is at the boundary, right = middle - 1 can skip past it, returning an index that excludes observers that should be notified. Fix: use the standard upper_bound pattern with right = size and right = middle (no -1), which correctly returns the count of elements whose targetJournalVersion <= value.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Fixes an off-by-one error in JournalObservable.upperBound() by switching to a standard upper_bound binary search variant, ensuring observers at boundary journal versions are correctly included.
Changes:
- Replaced inclusive-right binary search (
right = size - 1,right = middle - 1) with standardupper_bound(right = size,right = middle). - Removed fragile post-loop “fixup” logic and returned the computed insertion index directly.
- Updated method comments to describe the intended
upper_boundsemantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Return the number of elements whose targetJournalVersion <= value. | ||
| // This is the standard upper_bound pattern: returns the index of the first element > value. |
There was a problem hiding this comment.
The updated comment explains the return semantics, but the method still relies on important preconditions that aren’t stated: (1) array[0..size) must be sorted ascending by targetJournalVersion, and (2) the first size entries must be non-null JournalObservers. Adding these assumptions to the comment/Javadoc would make the method safer to reuse and easier to reason about.
| // Return the number of elements whose targetJournalVersion <= value. | |
| // This is the standard upper_bound pattern: returns the index of the first element > value. | |
| // Return the number of elements in array[0..size) whose targetJournalVersion <= value. | |
| // This is the standard upper_bound pattern: returns the index of the first element > value. | |
| // Preconditions: | |
| // - The first 'size' elements of 'array' are non-null JournalObserver instances. | |
| // - array[0..size) is sorted in ascending order by getTargetJournalVersion(). |
| // return min pos which is bigger than value | ||
| // Return the number of elements whose targetJournalVersion <= value. | ||
| // This is the standard upper_bound pattern: returns the index of the first element > value. | ||
| public static int upperBound(Object[] array, int size, Long value) { |
There was a problem hiding this comment.
The parameter name value is ambiguous for this domain (it represents a journal version threshold). Renaming it to something like journalVersion / targetJournalVersion would improve readability without affecting the method signature for callers.
| if (((JournalObserver) array[middle]).getTargetJournalVersion() <= value) { | ||
| left = middle + 1; | ||
| } else { | ||
| right = middle - 1; | ||
| right = middle; | ||
| } |
There was a problem hiding this comment.
Inside the loop, the cast and virtual call are embedded directly in the condition. Consider assigning (JournalObserver) array[middle] (and/or its targetJournalVersion) to a local variable for slightly clearer code and to avoid repeating complex expressions in hot paths.
Summary
Test plan
🤖 Generated with Claude Code