Skip to content
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

Make sure the LedgerHandle close callback can be completed when encounter exception #2913

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

codelipenghui
Copy link
Contributor

Related to apache/pulsar#12993

try {
errorOutPendingAdds(rc, pendingAdds);
} catch (Exception e) {
closePromise.completeExceptionally(e);
Copy link
Member

@zymap zymap Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there has an exception, do we still need to do the tearDownWriteHandleState() and MetadataUpdateLoop operations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it still needs the following operations. Catch this exception just to ensure the closePromise can complete.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't "completeExceptionally" or "complete" the closePromise twice

try {
errorOutPendingAdds(rc, pendingAdds);
} catch (Exception e) {
closePromise.completeExceptionally(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it still needs the following operations. Catch this exception just to ensure the closePromise can complete.

try {
errorOutPendingAdds(rc, pendingAdds);
} catch (Exception e) {
closePromise.completeExceptionally(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't "completeExceptionally" or "complete" the closePromise twice

errorOutPendingAdds(rc, pendingAdds);
try {
errorOutPendingAdds(rc, pendingAdds);
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about catching "Throwable" ?

@codelipenghui
Copy link
Contributor Author

@eolivelli Fixed, please check again.

Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@eolivelli eolivelli merged commit 05ca058 into apache:master Jul 29, 2022
@codelipenghui codelipenghui deleted the penghui/catch-exception branch July 29, 2022 06:58
zymap pushed a commit that referenced this pull request Aug 1, 2022
…nter exception (#2913)

* Make sure the LedgerHandle close callback can be completed when encounter exception.

(cherry picked from commit 05ca058)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
…nter exception (apache#2913)

* Make sure the LedgerHandle close callback can be completed when encounter exception.

(cherry picked from commit 05ca058)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
…nter exception (apache#2913)

* Make sure the LedgerHandle close callback can be completed when encounter exception.

(cherry picked from commit 05ca058)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
…nter exception (apache#2913)

* Make sure the LedgerHandle close callback can be completed when encounter exception.

(cherry picked from commit 05ca058)
(cherry picked from commit d8484b4)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…nter exception (apache#2913)

* Make sure the LedgerHandle close callback can be completed when encounter exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants