-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][ml] Retry offload reads when OffloadReadHandleClosedException is encountered #25148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix][ml] Retry offload reads when OffloadReadHandleClosedException is encountered #25148
Conversation
|
@codelipenghui Please add the following content to your PR description and select a checkbox: |
BewareMyPower
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to consider opening another PR for reset cursor change.
...d-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/cache/RangeEntryCacheImplTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/SubscriptionSeekTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/SubscriptionSeekTest.java
Outdated
Show resolved
Hide resolved
BewareMyPower
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a small style suggestion
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/RangeEntryCacheImpl.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #25148 +/- ##
=============================================
+ Coverage 37.41% 72.60% +35.19%
- Complexity 13148 33854 +20706
=============================================
Files 1899 1956 +57
Lines 150564 154739 +4175
Branches 17156 17644 +488
=============================================
+ Hits 56334 112349 +56015
+ Misses 86500 33420 -53080
- Partials 7730 8970 +1240
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Motivation
Resolve the OffloadReadHandleClosedException encountered by the client during cursor reset operations.
Modifications
When resetting a cursor to a timestamp that falls into offloaded data, the broker reads entries from the tiered storage. If the offloaded read handle is closed (e.g., due to tiered storage handle eviction or cleanup), the read operation fails with
OffloadReadHandleClosedException. The original code did not retry the read with a fresh handle, causing the reset operation to fail.Fix: Add retry logic in
RangeEntryCacheImpl.readFromStorage()to reopen the read handle and retry once whenOffloadReadHandleClosedExceptionis encountered.Note: The data consumption path already handles retries for the "Read handle closed" exception.
Verifying this change
testReadFromStorageRetriesWhenHandleClosedinRangeEntryCacheImplTest.javamvn -pl managed-ledger -Dtest=RangeEntryCacheImplTest testDocumentation
doc-not-needed