[improve][broker]Skip numOfEntriesToRead entries instead of skipping a ledger #17753
Conversation
lordcheng10
changed the title
lordcheng10
force-pushed
the
only_skip_not_exist_entrys
branch
from
425205e to
4a4f0a5
Compare
| PositionImpl skippedPosition; | ||
| try { | ||
| skippedPosition = getValidPositionInternal(position, skippedEntryNum); | ||
| } catch (NullPointerException e) { |
There was a problem hiding this comment.
Catching NPE is always a bad practice in Java
Please verify the cause of the NPE and add a null check
There was a problem hiding this comment.
OK , I will fix
There was a problem hiding this comment.
Fixed! PTAL,thanks! @eolivelli
| while (!isValidPosition(toPosition)) { | ||
| Long nextLedgerId = ledgers.ceilingKey(toPosition.getLedgerId() + 1); | ||
| if (nextLedgerId == null) { | ||
| throw new NullPointerException(); |
There was a problem hiding this comment.
Throwing NPE is bad practice.
Please throw a new checked exception.
This way we have full control over what we are catching
There was a problem hiding this comment.
Fixed! PTAL,thanks! @eolivelli
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
Show resolved
Hide resolved
| // try to find and move to next valid ledger | ||
| final Position nexReadPosition = cursor.getNextLedgerPosition(readPosition.getLedgerId()); | ||
| // Skip this read operation | ||
| PositionImpl nexReadPosition = cursor.ledger.getValidPositionAfterSkippedEntries(readPosition, count); |
There was a problem hiding this comment.
Please do not access a field, use a getter
There was a problem hiding this comment.
OK , I will fixed!
There was a problem hiding this comment.
Fixed! PTAL,thanks! @eolivelli
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/PositionImpl.java
Show resolved
Hide resolved
|
@codelipenghui @eolivelli @Jason918 @JipeiWang PTAL,thanks! |
|
CI all passed: lordcheng10#15 |
|
/pulsarbot run-failure-checks |
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
Show resolved
Hide resolved
lordcheng10
force-pushed
the
only_skip_not_exist_entrys
branch
from
6004b0e to
c1b1e5d
Compare
|
|
||
| public PositionImpl getValidPositionAfterSkippedEntries(final PositionImpl position, int skippedEntryNum) { | ||
| PositionImpl skippedPosition = position.getPositionAfterEntries(skippedEntryNum); | ||
| while (!isValidPosition(skippedPosition)) { |
There was a problem hiding this comment.
Is this same as getNextValidPosition(skippedPostion)?
There was a problem hiding this comment.
Fixed!
use getValidPositionAfterSkippedEntries for getNextValidPosition: @Jason918 PTAL,thanks!
|
/pulsarbot run-failure-checks |
|
/pulsarbot run-failure-checks |
Technoboy-
added
the
type/enhancement
Motivation
Skip numOfEntriesToRead entries instead of skipping a ledger.
When isAutoSkipNonRecoverableData=true, if a NonRecoverableLedgerException exception occurs when reading data, readPosition will automatically adjust to the next ledger to continue reading, which will result in a lot of existing data that cannot be read.
Modifications
Therefore, the entry skip logic is modified to: only skip numOfEntriesToRead entries instead of skipping a ledger
Documentation
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)