[fix][offload] Close all resources in BlobStoreBackedReadHandleImplV2.closeAsync#25296
[fix][offload] Close all resources in BlobStoreBackedReadHandleImplV2.closeAsync#25296iamsanjaymalakar wants to merge 2 commits intoapache:masterfrom
Conversation
|
@iamsanjaymalakar Please add the following content to your PR description and select a checkbox: |
.../java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImplV2.java
Outdated
Show resolved
Hide resolved
codelipenghui
left a comment
There was a problem hiding this comment.
And could you please also add tests to cover the changes from this PR, which can confirm the issue can be reproduced and avoid regression in the future.
|
Thanks for the reviews! I addressed the feedback by making |
|
Please run "Personal CI" to validate such changes on your side. You can run Pulsar CI in your personal fork by following these instructions: https://pulsar.apache.org/contribute/personal-ci/. It's free. |
|
One detail about BlobStoreBackedReadHandleImplV2 is that it's not used in Pulsar's production code. |
Motivation
BlobStoreBackedReadHandleImplV2#closeAsync()closes indices and dataStreams sequentially inside a single try/catch. If closing one resource throws, the method exits early and the remaining resources are never closed. This can leak file descriptors and leave resources open when offload read handles are closed.Modifications
Update
closeAsync()to attempt closing allOffloadIndexBlockV2instances and allDataInputStreams.Collect the first
IOExceptionand attach subsequent failures via suppressed exceptions.Complete the returned
CompletableFutureexceptionally if any close operation fails; otherwise mark the handle Closed and complete normally.docdoc-requireddoc-not-neededdoc-complete