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/03/16 14:24:48 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
poorbarcode opened a new pull request, #19841:
URL: https://github.com/apache/pulsar/pull/19841
### Motivation
The admin API `getSubscriptions` of `PersistentTopics` works like this:
1. getSubscriptions( `tp1` )
2. is partitioned topic?
`no`: return subscriptions
`yes`: getSubscriptions(`tp1-partition-0`)....getSubscriptions(`tp1-partition-n`)
If there is a bug that causes the broker to mistake `tp1-partition-n` for `tp1`, then an infinite loop of calls to `getSubscriptions( tp1 )` occurs(you can reproduce this issue by the test `testInfiniteHttpCallGetSubscriptions`).
<strong>(Highlight)</strong> This problem will be hit when a topic named like this `tp1-partition-0-DLQ-partition-0`, see:
https://github.com/apache/pulsar/blob/da788794ebe211bc96d4d961de49ffeeb473ffad/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java#L233-L237
### Modifications
Fix the wrong logic of the method `TopicName.getPartition(int index)`
### 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/78
--
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] codelipenghui commented on a diff in pull request #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#discussion_r1160358589
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
}
public TopicName getPartition(int index) {
- if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+ if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {
Review Comment:
What I thought was the PR intent to allow the partitioned name can contain `-partition-` keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.
--
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 wrong logic of method TopicName.getPartition(int index) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255560317
> @lhotari
>
> > > This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed
>
> > @poorbarcode In which case would this apply?
>
> See the section 4 in the Motivation
@poorbarcode Since #22705 has been cherry-picked to branch-3.0, does it prevent the problem from occuring?
--
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 wrong logic of method TopicName.getPartition(int index) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255454810
@poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.0.
--
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] wangjialing218 commented on pull request #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "wangjialing218 (via GitHub)" <gi...@apache.org>.
wangjialing218 commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-1477232751
This fix could solve #18149
--
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 #19841: [fix] [broker] Fix wrong logic of method TopicName.getPartition(int index)
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-1518945183
## [Codecov](https://codecov.io/gh/apache/pulsar/pull/19841?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 [#19841](https://codecov.io/gh/apache/pulsar/pull/19841?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (73cb806) into [master](https://codecov.io/gh/apache/pulsar/commit/6036dcce8d48f8e1ae311153eeb625f51c5aa827?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6036dcc) will **increase** coverage by `0.00%`.
> The diff coverage is `41.17%`.
[](https://codecov.io/gh/apache/pulsar/pull/19841?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 #19841 +/- ##
=========================================
Coverage 72.93% 72.93%
- Complexity 31934 31963 +29
=========================================
Files 1868 1868
Lines 138449 138458 +9
Branches 15235 15236 +1
=========================================
+ Hits 100978 100991 +13
Misses 29438 29438
+ Partials 8033 8029 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.19% <0.00%> (+0.08%)` | :arrow_up: |
| systests | `24.84% <0.00%> (-0.09%)` | :arrow_down: |
| unittests | `72.22% <41.17%> (+0.01%)` | :arrow_up: |
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/19841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pulsar/broker/admin/impl/PersistentTopicsBase.java](https://codecov.io/gh/apache/pulsar/pull/19841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1BlcnNpc3RlbnRUb3BpY3NCYXNlLmphdmE=) | `64.30% <33.33%> (-0.10%)` | :arrow_down: |
| [...mon/policies/data/stats/SubscriptionStatsImpl.java](https://codecov.io/gh/apache/pulsar/pull/19841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9wb2xpY2llcy9kYXRhL3N0YXRzL1N1YnNjcmlwdGlvblN0YXRzSW1wbC5qYXZh) | `69.91% <50.00%> (-0.74%)` | :arrow_down: |
| [...ava/org/apache/pulsar/common/naming/TopicName.java](https://codecov.io/gh/apache/pulsar/pull/19841?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvVG9waWNOYW1lLmphdmE=) | `96.33% <100.00%> (ø)` | |
... and [78 files with indirect coverage changes](https://codecov.io/gh/apache/pulsar/pull/19841/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 #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#discussion_r1160401769
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
}
public TopicName getPartition(int index) {
- if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+ if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {
Review Comment:
> we will not automatically create a partitioned topic for the DLQ topic even if users are enabled partitioned topic auto-creation.
Yes, [PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ]( https://github.com/apache/pulsar/issues/20033) try to do this.
--
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 wrong logic of method TopicName.getPartition(int index) [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255518969
@lhotari
>> This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed
> @poorbarcode In which case would this apply?
See the section 4 in the Motivation
<img width="903" alt="Screenshot 2024-07-29 at 18 04 12" src="https://github.com/user-attachments/assets/4d20a0cf-2854-4440-9b47-45ae420d5c81">
--
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 #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-1478851084
/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] poorbarcode commented on pull request #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-1513512129
I have submitted a quick fix for this issue. Please take a look. @Technoboy- @codelipenghui
- https://github.com/apache/pulsar/pull/20131
--
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 #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#discussion_r1160405999
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
}
public TopicName getPartition(int index) {
- if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+ if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {
Review Comment:
> What I thought was the PR intent to allow the partitioned name can contain -partition- keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.
I think there are three separate things:
- Our rule for a partition name and how to check which index it is: end with `-partition-x` or contain `-partition-x`
- The current logic is incorrect, and the current PR can fix it.
- Whether to allow to create a topic whose name contains `-paritition-`
- the behavior is denied for Admin API and is allowed for client(such as create by a cmd-subscribe), you can see the tests `testInfiniteHttpCallGetSubscriptions2`, `testInfiniteHttpCallGetSubscriptions3` in current PR
- Whether only create non-partitioned topics for the Dead Letter or Retry Letter
- [PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ]( https://github.com/apache/pulsar/issues/20033) try to do this.
--
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 #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#discussion_r1160405999
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
}
public TopicName getPartition(int index) {
- if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+ if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {
Review Comment:
> What I thought was the PR intent to allow the partitioned name can contain -partition- keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.
I think there are three things:
- Our rule for a partition name and how to check which index it is: end with `-partition-x` or contain `-partition-x`
- The current logic is incorrect, and current PR can fix it.
- Whether to allow to create a topic whose name contains `-paritition-`
- the behavior is denied for Admin API and is allowed for client(such as create by a cmd-subscribe), you can see the tests `testInfiniteHttpCallGetSubscriptions2`, `testInfiniteHttpCallGetSubscriptions3` in current PR
- Whether only create non-partitioned topics for the Dead Letter or Retry Letter
- [PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ]( https://github.com/apache/pulsar/issues/20033) try to do this.
--
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 #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-1477332086
Hi @wangjialing218
> This fix could solve https://github.com/apache/pulsar/issues/18149
Yes, I've seen the issue, and I'm sure it's the same one
--
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 wrong logic of method TopicName.getPartition(int index) [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255509613
> This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed
@poorbarcode In which case would this apply?
--
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 #19841: [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#discussion_r1160405999
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
}
public TopicName getPartition(int index) {
- if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+ if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {
Review Comment:
> What I thought was the PR intent to allow the partitioned name can contain -partition- keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.
I think there are three separate things:
- Our rule for a partition name and how to check which index it is: end with `-partition-x` or contain `-partition-x`
- The current logic is incorrect, and the current PR can fix it.
- Whether to allow to create a topic whose name contains `-paritition-`
- the behavior is denied for Admin API
-the behavior is allowed for client(such as create by a cmd-subscribe), you can see the tests `testInfiniteHttpCallGetSubscriptions2`, `testInfiniteHttpCallGetSubscriptions3` in current PR
- Whether only create non-partitioned topics for the Dead Letter or Retry Letter
- [PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ]( https://github.com/apache/pulsar/issues/20033) try to do this.
--
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- merged pull request #19841: [fix] [broker] Fix wrong logic of method TopicName.getPartition(int index)
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #19841:
URL: https://github.com/apache/pulsar/pull/19841
--
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 #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19841: [fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName
URL: https://github.com/apache/pulsar/pull/19841
--
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 wrong logic of method TopicName.getPartition(int index) [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #19841:
URL: https://github.com/apache/pulsar/pull/19841#issuecomment-2255484816
> @poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.0.
https://github.com/apache/pulsar/pull/19841/files#diff-93feae220f18ea80cb01ec7f2cefeab410aa0f3f181eed1206da4a294db0f701R233-R234 This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed
--
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