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 2021/11/17 05:15:00 UTC

[GitHub] [pulsar] michaeljmarshall opened a new pull request #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

michaeljmarshall opened a new pull request #12846:
URL: https://github.com/apache/pulsar/pull/12846


   …ers map
   
   ### Motivation
   
   There is currently a bug that allows a producer to be removed from a topic's `producers` map, but not from the `ServerCnx#producers` map. As a consequence, two producers with the same name can connect at the same time. 
   
   The fundamental issue here is with the definition of equality for the `Producer` class. This class's definition of equality has been an issue before. @merlimat fixed one such issue here: https://github.com/apache/pulsar/pull/11804. That fix addressed some of the problems, but I think we need to go a step further in making sure that the wrong producer cannot be removed from the topic's producers map.
   
   In the new test that I add with this PR, I show that it is still possible to remove the wrong producer. In the test, we essentially get two callbacks on the `getOrCreateTopic` future. Once that future succeeds, we create one producer and then the second fails and calls `producer.closeNow(true)`, which removes the successful producer from the topic. It does not remove the successful topic from the `ServerCnx` because of the changes introduced in https://github.com/apache/pulsar/pull/9256.
   
   The producer's `equals` method is only used when ensuring that the right producer is removed from the `producers` map and when determining if a producer is allowed to overwrite another producer. In my view, the simplest solution is to remove the overriding of `equals` and put the logic from `equals` into another method. This way removing a `producer` from a map is not too complicated (object equality is now reference equality), and we can still determine if a producer should override another.
   
   ### Modifications
   
   * Remove `Producer#equals` and `Producer#hashCode` to rely on the default object implementations.
   * Add `Producer#isSuccessorTo` to retain the logic required to determine if a producer is okay to overwrite an existing producer.
   
   ### Other Benefits
   
   When a producer is not present in a topic's producers map, the producer does not report metrics.
   
   ### Verifying this change
   
   I added a new test that shows a way to reproduce the issue. The new test fails on master but succeeds on my branch. There are also many tests that verify this code path to ensure this change does not introduce a regression.
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If `yes` was chosen, please highlight the changes*
   
     - Dependencies (does it add or upgrade a dependency): no
     - The public API: no
     - The schema: no
     - The default values of configurations: no
     - The wire protocol: no
     - The rest endpoints: no
     - The admin cli options: no
     - Anything that affects deployment: no
   
   ### Documentation
   
   - [x] `no-need-doc` 
     
   This is an internal bug fix.
   
   


-- 
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 merged pull request #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #12846:
URL: https://github.com/apache/pulsar/pull/12846


   


-- 
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 #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

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


   @ivankelly - in looking at https://github.com/apache/pulsar/pull/9256, I am wondering if I should also submit a PR similar to this one but for consumers. I haven't researched the consumer path for removal yet, so I cannot say for sure, but if there was a race for closing the wrong consumers, it's possible that the definition of equality is too broad for the `Consumer` class, and if so, the wrong consumer could be removed from the topic. Let me know, thanks.


-- 
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 #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

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



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java
##########
@@ -147,22 +147,17 @@ private String parseRemoteClusterName(String producerName, boolean isRemote) {
         return null;
     }
 
-    @Override

Review comment:
       The tests I've added/updated should fail if the definition of equality changes or becomes too broad. If you are worried that the tests are insufficient, I'm happy to add the overrides back in. Let me know.




-- 
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 #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

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


   /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] merlimat commented on a change in pull request #12846: [Broker] Fix producer getting incorrectly removed from topic's produc…

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



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java
##########
@@ -147,22 +147,17 @@ private String parseRemoteClusterName(String producerName, boolean isRemote) {
         return null;
     }
 
-    @Override

Review comment:
       Should we leave `hashCode()` & `equals()` with explanation on why it needs to be identity equal? Otherwise, someone will add it back later on... 




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