You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2023/04/10 02:19:21 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #20050: [fix] [ml] Fix uncompleted future when remove cursor
poorbarcode opened a new pull request, #20050:
URL: https://github.com/apache/pulsar/pull/20050
### Motivation
`SafeRunnable.safeRun(() -> callback.operationFailed(getException(ex)));` just create a runnable, but will not execute it.
### Modifications
do run the task
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository:
-
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #20050:
URL: https://github.com/apache/pulsar/pull/20050#discussion_r1161447439
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java:
##########
@@ -302,16 +302,17 @@ public void asyncRemoveCursor(String ledgerName, String cursorName, MetaStoreCal
}
callback.operationComplete(null, null);
}, executor.chooseThread(ledgerName))
- .exceptionallyAsync(ex -> {
- Throwable actEx = FutureUtil.unwrapCompletionException(ex);
- if (actEx instanceof MetadataStoreException.NotFoundException){
- log.info("[{}] [{}] cursor delete done because it did not exist.", ledgerName, cursorName);
- callback.operationComplete(null, null);
- return null;
- }
- SafeRunnable.safeRun(() -> callback.operationFailed(getException(ex)));
+ .exceptionally(ex -> {
+ executor.executeOrdered(ledgerName, SafeRunnable.safeRun(() -> {
+ Throwable actEx = FutureUtil.unwrapCompletionException(ex);
+ if (actEx instanceof MetadataStoreException.NotFoundException){
+ log.info("[{}] [{}] cursor delete done because it did not exist.", ledgerName, cursorName);
+ callback.operationComplete(null, null);
+ }
Review Comment:
Missing return statement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode closed pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode closed pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
URL: https://github.com/apache/pulsar/pull/20050
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on PR #20050:
URL: https://github.com/apache/pulsar/pull/20050#issuecomment-1501869814
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #20050:
URL: https://github.com/apache/pulsar/pull/20050#discussion_r1161413607
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java:
##########
@@ -309,7 +309,11 @@ public void asyncRemoveCursor(String ledgerName, String cursorName, MetaStoreCal
callback.operationComplete(null, null);
return null;
}
- SafeRunnable.safeRun(() -> callback.operationFailed(getException(ex)));
+ try {
+ callback.operationFailed(getException(ex));
+ } catch (Exception ex2){
+ log.error("Unexpected throwable caught when executing callback of async remove cursor", ex2);
+ }
return null;
}, executor.chooseThread(ledgerName));
Review Comment:
We should catch the exception thrown by `exceptionallyAsync`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20050:
URL: https://github.com/apache/pulsar/pull/20050#discussion_r1161502480
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java:
##########
@@ -302,16 +302,17 @@ public void asyncRemoveCursor(String ledgerName, String cursorName, MetaStoreCal
}
callback.operationComplete(null, null);
}, executor.chooseThread(ledgerName))
- .exceptionallyAsync(ex -> {
- Throwable actEx = FutureUtil.unwrapCompletionException(ex);
- if (actEx instanceof MetadataStoreException.NotFoundException){
- log.info("[{}] [{}] cursor delete done because it did not exist.", ledgerName, cursorName);
- callback.operationComplete(null, null);
- return null;
- }
- SafeRunnable.safeRun(() -> callback.operationFailed(getException(ex)));
+ .exceptionally(ex -> {
+ executor.executeOrdered(ledgerName, SafeRunnable.safeRun(() -> {
+ Throwable actEx = FutureUtil.unwrapCompletionException(ex);
+ if (actEx instanceof MetadataStoreException.NotFoundException){
+ log.info("[{}] [{}] cursor delete done because it did not exist.", ledgerName, cursorName);
+ callback.operationComplete(null, null);
+ }
Review Comment:
fixed. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #20050:
URL: https://github.com/apache/pulsar/pull/20050#discussion_r1161419606
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java:
##########
@@ -309,7 +309,11 @@ public void asyncRemoveCursor(String ledgerName, String cursorName, MetaStoreCal
callback.operationComplete(null, null);
return null;
}
- SafeRunnable.safeRun(() -> callback.operationFailed(getException(ex)));
+ try {
+ callback.operationFailed(getException(ex));
+ } catch (Exception ex2){
+ log.error("Unexpected throwable caught when executing callback of async remove cursor", ex2);
+ }
return null;
}, executor.chooseThread(ledgerName));
Review Comment:
As we discuss, we can push another pr to fix that, because all other methods are the same implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #20050: [fix] [ml] Fix uncompleted future when remove cursor
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #20050:
URL: https://github.com/apache/pulsar/pull/20050
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org