[GitHub] [activemq-artemis] cshannon commented on a change in pull request #3117: NO-JIRA Wrong key used for removal.

[GitBox <gi...@apache.org>]

cshannon commented on a change in pull request #3117:
URL: https://github.com/apache/activemq-artemis/pull/3117#discussion_r420673426



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/federation/address/FederatedAddress.java
##########
@@ -242,7 +242,7 @@ public void beforeRemoveBinding(SimpleString uniqueName, Transaction tx, boolean
                   if (entry.getKey().getDivert().getForwardAddress().equals(queue.getAddress())) {
                      final AddressInfo addressInfo = server.getPostOffice().getAddressInfo(binding.getAddress());
                      //check if the queue has been tracked by this divert and if so remove the consumer
-                     if (entry.getValue().remove(queue)) {
+                     if (entry.getValue().remove(queue.getAddress())) {

Review comment:
       Thanks for adding me @michaelandrepearce. Yes this is a feature I added and have been testing out a bit so I want to make sure it's working properly.
   
   After taking a quick 5 min look this seems that it could be a mistake since a SimpleString should be stored, however I don't think this is the right fix either. I believe the queue name is stored and not the address. 
   
   I am not back into the office until Monday but when I get back I will take a closer look at this to verify it is a mistake. Assuming it is wrong I will create a patch and some new tests to show it has been fixed.
   
   On another note, automated tools like error prone are great to help find errors. However, Clebert is right about needing a Jira and tests. Tests still need to be written to demonstrate the issue is fixed because I think in this case the fix is actually wrong as well and a test would show 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org