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 2024/02/05 09:31:42 UTC

[PR] [fix] [broker] Fix can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

poorbarcode opened a new pull request, #22025:
URL: https://github.com/apache/pulsar/pull/22025

   ### Motivation
   
   If a consumer tries to subscribe to a partitioned topic with a suffix-matched regexp, it does not work.
   - create topic `persistent://public/default/tp`
   - create a consumer with regexp `persistent://public/default/tp$`
   - the pattern consumer has no internal consumers.
   
   You can reproduce the issue by the test `testPreciseRegexpSubscribe`.
   - it can work when using `2.10.x`
   - it can not work when using `>=2.11.0`, it is a break change due to [PIP-145](https://github.com/apache/pulsar/issues/14505)
   
   https://github.com/apache/pulsar/pull/21885 fixed one wrong logic [here](https://github.com/apache/pulsar/pull/21885/files#diff-a36b69990c877e8ddc805bab84f15c3b119d42e152ba137d17e02e6909b0089cR54-R58), but there is still another wrong logic, see https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java#L64
   
   ```java
   @Override
   public void accept(String topicName, NotificationType notificationType) {
       // The param "topicName" contains the partition suffix, which causes a suffix-matched regexp not to match.
       if (topicsPattern.matcher(topicName).matches()) {
           List<String> newTopics;
           List<String> deletedTopics;
       ...
   ```
   
   ---
   
   ### Why can the test `testPreciseRegexpSubscribe` pass?
   https://github.com/apache/pulsar/pull/21885 added a test `testPreciseRegexpSubscribe` that covers the test which creates a consumer with a suffix-matched regexp. It works as below:
   - create a consumer with a suffix-matched regexp
   - create a partitioned topic
   - two events will trigger the pattern consumer to auto-create an internal single partition consumer.
     - Start a new `recheckTopicsChangeAfterReconnect` after the topic watcher is connected. https://github.com/apache/pulsar/pull/21885 fixed the wrong behavior.
     - Receive an event that a new topic created. Current PR is trying to fix the wrong behavior.
   
   The test `testPreciseRegexpSubscribe` can not pass as the below flow:
   - create a consumer with a suffix-matched regexp
   - wait for Topic List Watcher to start.
     - the consumer will call `recheckTopicsChangeAfterReconnect` at this step.
   - create a partitioned topic
     -  the consumer can not receive the event due to the wrong logic, then the test failed.
   
   ---
   
   ### Modifications
   - Review all the codes of the Topic List Watcher and add descriptions of the `topic name`.
   - Fix the wrong behavior.
   
   ---
   
   ### 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: x


-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#issuecomment-1951603470

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Comparison is base [(`fce0717`)](https://app.codecov.io/gh/apache/pulsar/commit/fce0717e52ec637a9430893dc5dc132b27368d80?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 72.90% compared to head [(`09410ea`)](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.58%.
   > Report is 22 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/22025/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #22025      +/-   ##
   ============================================
   + Coverage     72.90%   73.58%   +0.68%     
   - Complexity    32464    32562      +98     
   ============================================
     Files          1850     1874      +24     
     Lines        138611   139221     +610     
     Branches      15201    15260      +59     
   ============================================
   + Hits         101052   102448    +1396     
   + Misses        29711    28849     -862     
   - Partials       7848     7924      +76     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/22025/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/22025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.62% <0.00%> (?)` | |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/22025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.36% <0.00%> (?)` | |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/22025/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.85% <100.00%> (-0.05%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...apache/pulsar/broker/resources/TopicResources.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvcmVzb3VyY2VzL1RvcGljUmVzb3VyY2VzLmphdmE=) | `96.61% <ø> (ø)` | |
   | [...ache/pulsar/broker/namespace/NamespaceService.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9uYW1lc3BhY2UvTmFtZXNwYWNlU2VydmljZS5qYXZh) | `72.23% <ø> (+1.02%)` | :arrow_up: |
   | [...pulsar/broker/service/PulsarCommandSenderImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1B1bHNhckNvbW1hbmRTZW5kZXJJbXBsLmphdmE=) | `87.62% <ø> (ø)` | |
   | [...apache/pulsar/broker/service/TopicListService.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1RvcGljTGlzdFNlcnZpY2UuamF2YQ==) | `67.20% <100.00%> (ø)` | |
   | [...he/pulsar/client/impl/MultiTopicsConsumerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL011bHRpVG9waWNzQ29uc3VtZXJJbXBsLmphdmE=) | `78.08% <ø> (ø)` | |
   | [...ar/client/impl/PatternMultiTopicsConsumerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1BhdHRlcm5NdWx0aVRvcGljc0NvbnN1bWVySW1wbC5qYXZh) | `84.21% <ø> (+0.75%)` | :arrow_up: |
   | [...rg/apache/pulsar/client/impl/TopicListWatcher.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1RvcGljTGlzdFdhdGNoZXIuamF2YQ==) | `68.57% <ø> (+1.42%)` | :arrow_up: |
   | [...ar/client/impl/conf/ConsumerConfigurationData.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL2NvbmYvQ29uc3VtZXJDb25maWd1cmF0aW9uRGF0YS5qYXZh) | `92.55% <ø> (ø)` | |
   | [...va/org/apache/pulsar/common/protocol/Commands.java](https://app.codecov.io/gh/apache/pulsar/pull/22025?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9wcm90b2NvbC9Db21tYW5kcy5qYXZh) | `90.59% <ø> (ø)` | |
   
   ... and [209 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/22025/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </details>


-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1478134308


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java:
##########
@@ -681,16 +681,32 @@ public void testAutoSubscribePatterConsumerFromBrokerWatcher(boolean delayWatchi
         }
     }
 
-    @DataProvider(name= "partitioned")
-    public Object[][] partitioned(){
+    @DataProvider(name= "regexpConsumerArgs")
+    public Object[][] regexpConsumerArgs(){
         return new Object[][]{
-                {true},
-                {false}
+                {true, true},
+                {true, false},
+                {false, true},
+                {false, false}
         };
     }
 
-    @Test(timeOut = testTimeout, dataProvider = "partitioned")
-    public void testPreciseRegexpSubscribe(boolean partitioned) throws Exception {
+    private void waitForTopicListWatcherStarted(Consumer consumer) {
+        Awaitility.await().untilAsserted(() -> {
+            CompletableFuture completableFuture = WhiteboxImpl.getInternalState(consumer, "watcherFuture");
+            log.info("isDone: {}, isCompletedExceptionally: {}", completableFuture.isDone(),
+                    completableFuture.isCompletedExceptionally());
+            if (completableFuture.isCompletedExceptionally()) {

Review Comment:
   Seems can remove this `if`



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1478480573


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java:
##########
@@ -59,9 +65,12 @@ public List<String> getMatchingTopics() {
             return matchingTopics;
         }
 
+        /***
+         * @param topicName topic name which contains partition suffix.
+         */
         @Override
         public void accept(String topicName, NotificationType notificationType) {
-            if (topicsPattern.matcher(topicName).matches()) {
+            if (topicsPattern.matcher(TopicName.get(topicName).getPartitionedTopicName()).matches()) {

Review Comment:
   Could we check both the partitioned topic name and the topic name with the partition suffix?



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1483782209


##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java:
##########
@@ -681,16 +681,32 @@ public void testAutoSubscribePatterConsumerFromBrokerWatcher(boolean delayWatchi
         }
     }
 
-    @DataProvider(name= "partitioned")
-    public Object[][] partitioned(){
+    @DataProvider(name= "regexpConsumerArgs")
+    public Object[][] regexpConsumerArgs(){
         return new Object[][]{
-                {true},
-                {false}
+                {true, true},
+                {true, false},
+                {false, true},
+                {false, false}
         };
     }
 
-    @Test(timeOut = testTimeout, dataProvider = "partitioned")
-    public void testPreciseRegexpSubscribe(boolean partitioned) throws Exception {
+    private void waitForTopicListWatcherStarted(Consumer consumer) {
+        Awaitility.await().untilAsserted(() -> {
+            CompletableFuture completableFuture = WhiteboxImpl.getInternalState(consumer, "watcherFuture");
+            log.info("isDone: {}, isCompletedExceptionally: {}", completableFuture.isDone(),
+                    completableFuture.isCompletedExceptionally());
+            if (completableFuture.isCompletedExceptionally()) {

Review Comment:
   Removed



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1478463545


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java:
##########
@@ -59,9 +65,12 @@ public List<String> getMatchingTopics() {
             return matchingTopics;
         }
 
+        /***
+         * @param topicName topic name which contains partition suffix.
+         */
         @Override
         public void accept(String topicName, NotificationType notificationType) {
-            if (topicsPattern.matcher(topicName).matches()) {
+            if (topicsPattern.matcher(TopicName.get(topicName).getPartitionedTopicName()).matches()) {

Review Comment:
   The problem is the topic name with a partition suffix can't match the subscription regexp topic name, right?



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1478463545


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java:
##########
@@ -59,9 +65,12 @@ public List<String> getMatchingTopics() {
             return matchingTopics;
         }
 
+        /***
+         * @param topicName topic name which contains partition suffix.
+         */
         @Override
         public void accept(String topicName, NotificationType notificationType) {
-            if (topicsPattern.matcher(topicName).matches()) {
+            if (topicsPattern.matcher(TopicName.get(topicName).getPartitionedTopicName()).matches()) {

Review Comment:
   The problem is the topic name with a partition suffix can't match the subscription regexp topic name, right?



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #22025:
URL: https://github.com/apache/pulsar/pull/22025#discussion_r1483784690


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java:
##########
@@ -59,9 +65,12 @@ public List<String> getMatchingTopics() {
             return matchingTopics;
         }
 
+        /***
+         * @param topicName topic name which contains partition suffix.
+         */
         @Override
         public void accept(String topicName, NotificationType notificationType) {
-            if (topicsPattern.matcher(topicName).matches()) {
+            if (topicsPattern.matcher(TopicName.get(topicName).getPartitionedTopicName()).matches()) {

Review Comment:
   @gaoran10 Ah, you means return `true` whether `{tenant}/{ns}/{tp}` is matches or `{tenant}/{ns}/{tp}-{partition}` is matches. Good point. We can submit a new PIP to do this(it is a behavior change)
   
   This PR is in order to revet the behavior as the original behavior before [PIP-145](https://github.com/apache/pulsar/issues/14505)(it did a break change).



-- 
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 can not subscribe partitioned topic with a suffix-matched regexp [pulsar]

Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #22025:
URL: https://github.com/apache/pulsar/pull/22025


-- 
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