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/05 06:44:33 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #19858: [fix][client] Release the orphan producers after the primary consumer is closed
poorbarcode opened a new pull request, #19858:
URL: https://github.com/apache/pulsar/pull/19858
### Motivation
The producers ["retryLetterProducer", "deadLetterProducer"] will be auto-created by consumers if enabled `DLQ`, but these producers will not close after consumers are closed.
### Modifications
Auto close "retryLetterProducer" and "deadLetterProducer" after the primary consumer is closed
### 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:
- https://github.com/poorbarcode/pulsar/pull/79
--
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 #19858: [fix][client] Release the orphan producers after the primary consumer is closed
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19858:
URL: https://github.com/apache/pulsar/pull/19858#issuecomment-1497073947
## [Codecov](https://codecov.io/gh/apache/pulsar/pull/19858?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 [#19858](https://codecov.io/gh/apache/pulsar/pull/19858?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (54ad9d7) into [master](https://codecov.io/gh/apache/pulsar/commit/f20dc93bc7d3defcc824857a558f2f78726f13a1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f20dc93) will **increase** coverage by `13.05%`.
> The diff coverage is `87.50%`.
[](https://codecov.io/gh/apache/pulsar/pull/19858?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 #19858 +/- ##
=============================================
+ Coverage 25.11% 38.16% +13.05%
- Complexity 205 1364 +1159
=============================================
Files 1680 1686 +6
Lines 127176 133729 +6553
Branches 13863 15400 +1537
=============================================
+ Hits 31942 51040 +19098
+ Misses 90242 75961 -14281
- Partials 4992 6728 +1736
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `25.36% <87.50%> (?)` | |
| systests | `25.17% <21.42%> (+0.05%)` | :arrow_up: |
| unittests | `33.13% <75.00%> (?)` | |
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/19858?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/pulsar/common/util/URIPreconditions.java](https://codecov.io/gh/apache/pulsar/pull/19858?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi91dGlsL1VSSVByZWNvbmRpdGlvbnMuamF2YQ==) | `44.44% <0.00%> (+44.44%)` | :arrow_up: |
| [...va/org/apache/pulsar/client/impl/ConsumerImpl.java](https://codecov.io/gh/apache/pulsar/pull/19858?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVySW1wbC5qYXZh) | `57.06% <93.33%> (+29.06%)` | :arrow_up: |
... and [581 files with indirect coverage changes](https://codecov.io/gh/apache/pulsar/pull/19858/indirect-changes?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] poorbarcode commented on a diff in pull request #19858: [fix] [cli] Release the orphan producers after the primary consumer is closed
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19858:
URL: https://github.com/apache/pulsar/pull/19858#discussion_r1143503506
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -1050,7 +1050,19 @@ public CompletableFuture<Void> closeAsync() {
});
}
- return closeFuture;
+ return closeFuture.thenCompose(ignore -> {
+ if (retryLetterProducer != null){
+ return retryLetterProducer.closeAsync();
Review Comment:
> Should we close them parallelly?
Fixed.
> Do you think we should make some special exception handling here?
Yes, if closing these producers fail, users can not retry to close them again, so just print a warning log. Or two other implementations:
- Make this method retryable.
- Do retry these producers' close silently
Since there are few scenarios where failure occurs, such as when the connection has been broken (no longer necessary to retry the close), just print a warning log.
--
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 #19858: [fix] [cli] Release the orphan producers after the primary consumer is closed
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19858: [fix] [cli] Release the orphan producers after the primary consumer is closed
URL: https://github.com/apache/pulsar/pull/19858
--
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] lhotari closed pull request #19858: [fix][client] Release the orphan producers after the primary consumer is closed
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari closed pull request #19858: [fix][client] Release the orphan producers after the primary consumer is closed
URL: https://github.com/apache/pulsar/pull/19858
--
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 merged pull request #19858: [fix][client] Release the orphan producers after the primary consumer is closed
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #19858:
URL: https://github.com/apache/pulsar/pull/19858
--
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 #19858: [fix] [cli] Release the orphan producers after the primary consumer is closed
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19858:
URL: https://github.com/apache/pulsar/pull/19858#discussion_r1143424835
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -1050,7 +1050,19 @@ public CompletableFuture<Void> closeAsync() {
});
}
- return closeFuture;
+ return closeFuture.thenCompose(ignore -> {
+ if (retryLetterProducer != null){
+ return retryLetterProducer.closeAsync();
Review Comment:
Should we close them parallelly? Do you think we should make some special exception handling 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] poorbarcode commented on pull request #19858: [fix][client] Release the orphan producers after the primary consumer is closed
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19858:
URL: https://github.com/apache/pulsar/pull/19858#issuecomment-1497433897
/pulsarbot rerun-failure-checks
--
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 #19858: [fix] [cli] Release the orphan producers after the primary consumer is closed
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #19858:
URL: https://github.com/apache/pulsar/pull/19858#discussion_r1143424835
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -1050,7 +1050,19 @@ public CompletableFuture<Void> closeAsync() {
});
}
- return closeFuture;
+ return closeFuture.thenCompose(ignore -> {
+ if (retryLetterProducer != null){
+ return retryLetterProducer.closeAsync();
Review Comment:
Should we close them parallel? should we do some special exception handling 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