[fix][broker][PIP-195] fix cursor skip read #19124
Conversation
coderzc
changed the title
coderzc
force-pushed
the
fix_cursor_read_skip
branch
from
4a4a1a4 to
f1a7bd7
Compare
Codecov Report
@@ Coverage Diff @@
## master #19124 +/- ##
============================================
+ Coverage 47.43% 47.70% +0.27%
- Complexity 9535 9594 +59
============================================
Files 632 632
Lines 59839 59839
Branches 6234 6234
============================================
+ Hits 28384 28548 +164
+ Misses 28375 28194 -181
- Partials 3080 3097 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| completableFuture.complete(entries.size()); | ||
| try { | ||
| entries.forEach(entry -> { | ||
| assertNotEquals(entry.getEntryId() % 2, 0); |
There was a problem hiding this comment.
It should be assertEquals(entry.getEntryId() % 2, 0);
And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
There was a problem hiding this comment.
And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
I already use completableFuture.completeExceptionally(e) to receive exception, it should be can work.
But AssertionError is not a subclass of Exception, so we should be caught Throwable.
| completableFuture2.complete(entries.size()); | ||
| try { | ||
| entries.forEach(entry -> { | ||
| assertEquals(entry.getEntryId() % 2, 0); |
There was a problem hiding this comment.
Same as the above comment. You might need to consider using a different way to assert the result.
| long entryId = firstEntry; | ||
| for (; entryId <= lastEntry; entryId++) { | ||
| if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) { | ||
| if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) { |
There was a problem hiding this comment.
I suggest Invert if condition to help simple understand this logic. : )
| } | ||
| } | ||
|
|
||
| if (firstValidEntry != -1L) { |
There was a problem hiding this comment.
It looks like this logic just works for the else branch. Can we merge it?
There was a problem hiding this comment.
Yes, we can move this logic to the else branch
coderzc
force-pushed
the
fix_cursor_read_skip
branch
from
5dc2bcc to
a32996f
Compare
4 participants
PIP: #16763
Motivation
In #19035, the
skipConditionparameter returns need to skip entry when reading entry, but the logic now is reversed.Modifications
Fix skip logic and add test to cover it.
Verifying this change
testReadEntriesWithSkip
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: