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/03/31 09:55:42 UTC

[GitHub] [pulsar] lordcheng10 opened a new pull request #14970: Fix:The offline consumer is not removed from the consumerList

lordcheng10 opened a new pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970


   
   ### Motivation
   
   We found a problem: when the consumer was removed, it was not removed from the consumerList, but it was removed from the consumerSet and consumers:
   <img width="1143" alt="image" src="https://user-images.githubusercontent.com/19296967/161028959-66f6a743-d26f-4eb0-b90c-fccd7d7940a5.png">
   
   
   ### Documentation
   
   Check the box below or label this PR directly (if you have committer privilege).
   
   Need to update docs? 
   
   - [ ] `doc-required` 
     
     (If you need help on updating docs, create a doc issue)
     
   - [x] `no-need-doc` 
     
     (Please explain why)
     
   - [ ] `doc` 
     
     (If this PR contains doc changes)
   
   
   


-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840473676



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,13 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {

Review comment:
       > Can you provide the case of duplicated consumer?
   
   OK, I will try to find duplicate cases.




-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941600


   
   I think we also need to implement a method similar to isSuccessorTo to determine whether the consumer can be overridden, what do you think? @lhotari @michaeljmarshall @eolivelli 


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR :
   https://github.com/apache/pulsar/pull/12846#issuecomment-971200579
   


-- 
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] lordcheng10 commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them?
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR https://github.com/apache/pulsar/pull/12846, and in our scenario, it does seem to remove the wrong consumer.


-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840189904



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       Fixed:
   1.do a check in addConsumer;
   2.use removeIf;
   
   PTAL,thanks! @lhotari @eolivelli 




-- 
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] lordcheng10 commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1085583348


   
   /pulsarbot run-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] lhotari commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840258044



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       @lordcheng10 there's a minor checkstyle issue in the recent changes. please check that




-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840293804



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,14 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {
+            log.warn("[{}] Consumer with the same id is already created:"
+                            + " consumerId={}, consumer={}",
+                    consumer.cnx().clientAddress(), consumer.consumerId(), consumer);
+            throw new BrokerServiceException("Consumer with the same id is already created!");

Review comment:
       Fixed.




-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840189904



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       Fixed:
   1.do a check in addConsumer;
   2.use removeIf;
   
   PTAL,thanks! @lhotari 




-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


-- 
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 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r839825039



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       The `removeAllConsumer` doesn't look great. I wonder if using `.removeIf` would be better:
   ```suggestion
               consumerList.removeIf(consumer::equals);
   ```




-- 
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] lordcheng10 commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1085759704


   /pulsarbot run-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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840280963



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       Fixed.
   PTAL,thanks! @lhotari 




-- 
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] xuesongxs removed a comment on pull request #14970: Fix:The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
xuesongxs removed a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084447981


   https://github.com/apache/pulsar/issues/13787


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   <img width="1335" alt="image" src="https://user-images.githubusercontent.com/19296967/161125415-3172dbf7-3106-4fbf-a801-0d80e8108362.png">
   
   <img width="1547" alt="image" src="https://user-images.githubusercontent.com/19296967/161037021-eea6e429-e38d-4baa-adf7-07e80dce4e28.png">
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,:
   <img width="1001" alt="image" src="https://user-images.githubusercontent.com/19296967/161124313-9d3a7da0-3686-4699-8ddc-2d698689688e.png">
   


-- 
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] michaeljmarshall commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r839835383



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       I agree with this sentiment. I wonder about the data structure itself, too. Why is it a list and not a set?




-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR https://github.com/apache/pulsar/pull/12846, and in our scenario, it does seem to remove the wrong consumer.


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In our scenario, we also found a problem: when the consumer reconnects, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers. The wrong consumer could be removed . 
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   <img width="1335" alt="image" src="https://user-images.githubusercontent.com/19296967/161125415-3172dbf7-3106-4fbf-a801-0d80e8108362.png">
   
   <img width="1547" alt="image" src="https://user-images.githubusercontent.com/19296967/161037021-eea6e429-e38d-4baa-adf7-07e80dce4e28.png">
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   
   


-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r839883891



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       I agree




-- 
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 change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840281674



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,14 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {
+            log.warn("[{}] Consumer with the same id is already created:"
+                            + " consumerId={}, consumer={}",
+                    consumer.cnx().clientAddress(), consumer.consumerId(), consumer);
+            throw new BrokerServiceException("Consumer with the same id is already created!");

Review comment:
       what about using ConsumerBusyException ?

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,14 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {
+            log.warn("[{}] Consumer with the same id is already created:"
+                            + " consumerId={}, consumer={}",
+                    consumer.cnx().clientAddress(), consumer.consumerId(), consumer);
+            throw new BrokerServiceException("Consumer with the same id is already created!");
+

Review comment:
       nit: remove useless newline




-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   <img width="1335" alt="image" src="https://user-images.githubusercontent.com/19296967/161125415-3172dbf7-3106-4fbf-a801-0d80e8108362.png">
   
   <img width="1547" alt="image" src="https://user-images.githubusercontent.com/19296967/161037021-eea6e429-e38d-4baa-adf7-07e80dce4e28.png">
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941600


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them? 
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR https://github.com/apache/pulsar/pull/12846, and in our scenario, it does seem to remove the wrong consumer.
   
   I think we also need to implement a method similar to isSuccessorTo to determine whether the consumer can be overridden, what do you think? @lhotari @michaeljmarshall @eolivelli 


-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r839883891



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       I agree  to @lhotari 's suggestion




-- 
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] xuesongxs commented on pull request #14970: Fix:The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
xuesongxs commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084447981


   https://github.com/apache/pulsar/issues/13787


-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840472131



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,13 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {

Review comment:
       Yes so I think we should write a method like org.apache.pulsar.broker.service.Producer#isSuccessorTo to judge whether the previous consumer object should be overwritten by consumerEpoch,like this:
   
   public boolean isSuccessorTo(Consumer other){
       return Objects.equals(cnx.clientAddress(), other.cnx.clientAddress()) && consumerId == other.consumerId
               &&  other.consumerEpoch < consumerEpoch;
   }




-- 
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] Jason918 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840406213



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,13 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {

Review comment:
       Can you provide the case of duplicated consumer? 
   I wonder if we should replace the old consumer with the new one. The older consumer maybe somewhat expired.




-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840472131



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,13 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {

Review comment:
       Yes so I think we should write a method like org.apache.pulsar.broker.service.Producer#isSuccessorTo to judge whether the previous consumer object should be overwritten by consumerEpoch,like this:
   
   public boolean isSuccessorTo(Consumer other){
         return Objects.equals(cnx.clientAddress(), other.cnx.clientAddress()) && consumerId == other.consumerId
                 &&  other.consumerEpoch < consumerEpoch;
   }




-- 
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] lordcheng10 commented on a change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r840293910



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -153,6 +153,14 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
             throw new ConsumerBusyException("Subscription reached max consumers limit");
         }
 
+        if (consumerList.contains(consumer)) {
+            log.warn("[{}] Consumer with the same id is already created:"
+                            + " consumerId={}, consumer={}",
+                    consumer.cnx().clientAddress(), consumer.consumerId(), consumer);
+            throw new BrokerServiceException("Consumer with the same id is already created!");
+

Review comment:
       Fixed. PTAL,thanks! @eolivelli 




-- 
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] lordcheng10 commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941600


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them? 
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR https://github.com/apache/pulsar/pull/12846, and in our scenario, it does seem to remove the wrong consumer.


-- 
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 change in pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#discussion_r839841300



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java
##########
@@ -163,12 +163,17 @@ protected boolean isConsumersExceededOnSubscription() {
         return isConsumersExceededOnSubscription(topic, consumerList.size());
     }
 
+    private void removeAllConsumer(Consumer consumer){
+        while(consumerList.remove(consumer)){
+        }
+    }
+    
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
         // decrement unack-message count for removed consumer
         addUnAckedMessages(-consumer.getUnackedMessages());
         if (consumerSet.removeAll(consumer) == 1) {
-            consumerList.remove(consumer);
+            removeAllConsumer(consumer);

Review comment:
       +1 to @lhotari 's suggestion




-- 
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] michaeljmarshall commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084982301


   @lordcheng10 - that is a great finding. I think it seems very reasonable to adjust the consumer logic, but I still haven't researched the consumer remove logic, so I am not authoritative on the subject. I wonder if we'd be able to add a test that shows the `isSuccessorTo` logic is necessary.


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In our scenario, we also found a problem: when the consumer reconnects, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers. The wrong consumer could be removed . 
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   <img width="1335" alt="image" src="https://user-images.githubusercontent.com/19296967/161125415-3172dbf7-3106-4fbf-a801-0d80e8108362.png">
   
   <img width="1547" alt="image" src="https://user-images.githubusercontent.com/19296967/161037021-eea6e429-e38d-4baa-adf7-07e80dce4e28.png">
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


-- 
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] lordcheng10 commented on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1085567524


   
   /pulsarbot run-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] xuesongxs commented on pull request #14970: Fix:The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
xuesongxs commented on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084449056


   https://github.com/apache/pulsar/issues/13787
   Just like this bug.


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941600


   I think we also need to implement a method similar to isSuccessorTo to determine whether the consumer can be overridden, what do you think? @lhotari @michaeljmarshall @eolivelli 
   
   like this:
   
   
       public boolean isSuccessorTo(Consumer other){
           return Objects.equals(cnx.clientAddress(), other.cnx.clientAddress()) && consumerId == other.consumerId
                   &&  other.consumerEpoch < consumerEpoch;
       }


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   <img width="787" alt="image" src="https://user-images.githubusercontent.com/19296967/161125305-faacaad2-7b7c-4d86-8164-85514603355b.png">
   
   <img width="1335" alt="image" src="https://user-images.githubusercontent.com/19296967/161125415-3172dbf7-3106-4fbf-a801-0d80e8108362.png">
   
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


-- 
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] lordcheng10 edited a comment on pull request #14970: [fix][broker] The offline consumer is not removed from the consumerList

Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #14970:
URL: https://github.com/apache/pulsar/pull/14970#issuecomment-1084941134


   In fact, the problem here may be the problem of consumer equality, similar to the previous producer problem.
   
   When there are duplicate consumers in the consumerList, it is possible to remove the actually successfully registered consumers when removing them.
   
   In fact, in our scenario, we also found a problem: when the consumer reconnects, although we can see that the number of consumers has not changed through monitoring, the broker no longer pushes messages to these consumers. Through the stats command, we see these The consumer's permit is 0, and it is a consumer that has been removed from the consumerSet and consumers.
   
   
   So the fundamental problem here is actually the definition of consumer equality. I think we might need to do something like https://github.com/apache/pulsar/pull/12846 on the definition of equality for consumers.
   
   I see concerns about the definition of consumer equality in this PR ,and this worry seems right:
   <img width="944" alt="image" src="https://user-images.githubusercontent.com/19296967/161124672-1c45b767-f4af-42f9-aa77-40707a008818.png">
   
   


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