You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/12 10:36:28 UTC
[GitHub] [pulsar] tjiuming opened a new pull request, #18886: [fix][broker] Fix PendingAckHandleImpl when relay failed.
tjiuming opened a new pull request, #18886:
URL: https://github.com/apache/pulsar/pull/18886
### Motivation
Currently, we don't handle PendingAckHandleImpl relay failed.
If the operation failed, it won't relay again, and won't remove the subscription from PersistentTopic which is already created.
It will lead to an unexpected situation: we couldn't create Consumers on the subscription.
Because in PersistentTopic#subscriptions, the subscription already exists, but initialized failed.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `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: <!-- ENTER URL HERE -->
--
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] eolivelli commented on a diff in pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#discussion_r1091922347
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java:
##########
@@ -890,6 +901,23 @@ public CompletableFuture<Void> close() {
}
}
+ protected void retryClose() {
+ ScheduledExecutorService scheduler = topic.getBrokerService().pulsar().getExecutor();
+ scheduler.schedule(() -> {
+ close()
+ .thenAccept(unused -> {
+ if (log.isDebugEnabled()) {
+ log.debug("Close subscription {} finished.", subName);
+ }
+ })
+ .exceptionally(t -> {
+ log.warn("Close subscription {} failed.", subName, t);
+ retryClose();
+ return null;
+ });
+ }, 20, TimeUnit.SECONDS);
Review Comment:
20 seconds ?
Probably we have to log at WARN level something like
log.warn("Re-scheduling closing of subscription {} in 20 seconds")
--
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] tjiuming commented on a diff in pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
Posted by "tjiuming (via GitHub)" <gi...@apache.org>.
tjiuming commented on code in PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#discussion_r1099741514
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -283,12 +283,22 @@ public PersistentTopic(String topic, ManagedLedger ledger, BrokerService brokerS
// ignore it for now and let the message dedup logic to take care of it
} else {
final String subscriptionName = Codec.decode(cursor.getName());
- subscriptions.put(subscriptionName, createPersistentSubscription(subscriptionName, cursor,
+ PersistentSubscription subscription = createPersistentSubscription(subscriptionName, cursor,
PersistentSubscription.isCursorFromReplicatedSubscription(cursor),
- cursor.getCursorProperties()));
- // subscription-cursor gets activated by default: deactivate as there is no active subscription right
- // now
- subscriptions.get(subscriptionName).deactivateCursor();
+ cursor.getCursorProperties());
+ subscriptions.put(subscriptionName, subscription);
+ subscription.getInitializeFuture()
+ .exceptionally(t -> {
+ log.warn("PersistentSubscription [{}] pendingAckHandleImpl relay failed "
+ + "when initialize topic [{}].", subscriptionName, topic, t);
+ if (subscriptions.remove(subscriptionName, subscription)) {
+ subscription.retryClose();
Review Comment:
I think it may happen.
```
subscription.getInitializeFuture()
.exceptionally(t -> {
// ignore...
})
```
will be executed after the topic created. If a consumer connected the topic and unsubscribe the topic, `subscriptions.remove(...)` may return false.
--
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] github-actions[bot] commented on pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#issuecomment-1396346055
The pr had no activity for 30 days, mark with Stale label.
--
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
Re: [PR] [fix][broker] Fix PendingAckHandleImpl when replay failed. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun closed pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
URL: https://github.com/apache/pulsar/pull/18886
--
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
Re: [PR] [fix][broker] Fix PendingAckHandleImpl when replay failed. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#issuecomment-1863414679
Will be fixed by https://github.com/apache/pulsar/pull/21760
--
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] codecov-commenter commented on pull request #18886: [fix][broker] Fix PendingAckHandleImpl when relay failed.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#issuecomment-1346797997
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18886?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18886](https://codecov.io/gh/apache/pulsar/pull/18886?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f81902b) into [master](https://codecov.io/gh/apache/pulsar/commit/1b5722d35a2cbce35d1d0843fa6a999da5feb583?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1b5722d) will **decrease** coverage by `4.62%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/pulsar/pull/18886?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18886 +/- ##
============================================
- Coverage 46.34% 41.72% -4.63%
+ Complexity 10394 1067 -9327
============================================
Files 703 80 -623
Lines 68838 9752 -59086
Branches 7379 1236 -6143
============================================
- Hits 31905 4069 -27836
+ Misses 33324 5110 -28214
+ Partials 3609 573 -3036
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `41.72% <85.71%> (-4.63%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18886?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ookkeeper/mledger/impl/ManagedCursorContainer.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JDb250YWluZXIuamF2YQ==) | `76.80% <85.71%> (+0.76%)` | :arrow_up: |
| [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | `37.06% <0.00%> (-0.06%)` | :arrow_down: |
| [...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRMZWRnZXJJbXBsLmphdmE=) | `53.95% <0.00%> (-0.05%)` | :arrow_down: |
| [...r/service/AbstractDispatcherMultipleConsumers.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0RGlzcGF0Y2hlck11bHRpcGxlQ29uc3VtZXJzLmphdmE=) | | |
| [...ava/org/apache/pulsar/broker/service/Consumer.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0NvbnN1bWVyLmphdmE=) | | |
| [...ker/service/persistent/PersistentSubscription.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFN1YnNjcmlwdGlvbi5qYXZh) | | |
| [...sar/broker/service/persistent/PersistentTopic.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFRvcGljLmphdmE=) | | |
| [...nt/impl/PulsarClientImplementationBindingImpl.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1B1bHNhckNsaWVudEltcGxlbWVudGF0aW9uQmluZGluZ0ltcGwuamF2YQ==) | | |
| [...che/pulsar/client/impl/schema/LocalTimeSchema.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL3NjaGVtYS9Mb2NhbFRpbWVTY2hlbWEuamF2YQ==) | | |
| [...sar/broker/loadbalance/impl/PulsarServiceUnit.java](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL1B1bHNhclNlcnZpY2VVbml0LmphdmE=) | | |
| ... and [619 more](https://codecov.io/gh/apache/pulsar/pull/18886/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
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] tjiuming commented on pull request #18886: [fix][broker] Fix PendingAckHandleImpl when relay failed.
Posted by GitBox <gi...@apache.org>.
tjiuming commented on PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#issuecomment-1346247804
Tests to be completed
--
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- closed pull request #18886: [fix][broker] Fix PendingAckHandleImpl when relay failed.
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18886: [fix][broker] Fix PendingAckHandleImpl when relay failed.
URL: https://github.com/apache/pulsar/pull/18886
--
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] congbobo184 commented on a diff in pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
Posted by "congbobo184 (via GitHub)" <gi...@apache.org>.
congbobo184 commented on code in PR #18886:
URL: https://github.com/apache/pulsar/pull/18886#discussion_r1098781763
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -283,12 +283,22 @@ public PersistentTopic(String topic, ManagedLedger ledger, BrokerService brokerS
// ignore it for now and let the message dedup logic to take care of it
} else {
final String subscriptionName = Codec.decode(cursor.getName());
- subscriptions.put(subscriptionName, createPersistentSubscription(subscriptionName, cursor,
+ PersistentSubscription subscription = createPersistentSubscription(subscriptionName, cursor,
PersistentSubscription.isCursorFromReplicatedSubscription(cursor),
- cursor.getCursorProperties()));
- // subscription-cursor gets activated by default: deactivate as there is no active subscription right
- // now
- subscriptions.get(subscriptionName).deactivateCursor();
+ cursor.getCursorProperties());
+ subscriptions.put(subscriptionName, subscription);
+ subscription.getInitializeFuture()
+ .exceptionally(t -> {
+ log.warn("PersistentSubscription [{}] pendingAckHandleImpl relay failed "
+ + "when initialize topic [{}].", subscriptionName, topic, t);
+ if (subscriptions.remove(subscriptionName, subscription)) {
+ subscription.retryClose();
Review Comment:
why should add this method? persistentTopic init sub then remove will return false?
--
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