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

Future completed twice in the method of impl.MLPendingAckStore#closeAsync #12362

Merged
merged 4 commits into from
Oct 22, 2021

Conversation

lordcheng10
Copy link
Contributor

@lordcheng10 lordcheng10 commented Oct 14, 2021

CompletableFuture is completed twice in this code:

cursor.asyncClose(new AsyncCallbacks.CloseCallback() {
@Override
public void closeComplete(Object ctx) {
try {
managedLedger.close();
} catch (Exception e) {
completableFuture.completeExceptionally(e);
}
completableFuture.complete(null);
}
@Override
public void closeFailed(ManagedLedgerException exception, Object ctx) {
completableFuture.completeExceptionally(exception);
}
}, null);

@lordcheng10 lordcheng10 changed the title Future completed twice. Future completed twice in the method of impl.MLPendingAckStore#closeAsync Oct 14, 2021
@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Oct 14, 2021

@congbobo184 As you mentioned in #12113 (comment), I fixed it here

@congbobo184
Copy link
Contributor

@lordcheng10 could you please change managedLedger.close to managedLedger.closeAsync and add some log for it. thanks.

@lordcheng10
Copy link
Contributor Author

@lordcheng10 could you please change managedLedger.close to managedLedger.closeAsync and add some log for it. thanks.

Ok, will fix!

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Oct 14, 2021

@lordcheng10 could you please change managedLedger.close to managedLedger.closeAsync and add some log for it. thanks.

please review again. thanks. @congbobo184

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Anonymitaet
Copy link
Member

@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Oct 18, 2021

@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

I think there is no need to modify the document, this PR did not change the original logic.

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Oct 18, 2021
@lordcheng10
Copy link
Contributor Author

@merlimat PTAL,thanks!

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.

+1

@eolivelli eolivelli merged commit 10371ee into apache:master Oct 22, 2021
@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug area/broker labels Oct 22, 2021
@lhotari
Copy link
Member

lhotari commented Oct 22, 2021

Good work @lordcheng10 . It seems that this fix also has a performance improvement aspect since it replaces a blocking close method with closeAsync. Nice.

codelipenghui pushed a commit that referenced this pull request Oct 26, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 26, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
codelipenghui pushed a commit that referenced this pull request Dec 20, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants