[jira] [Created] (AMQ-5474) Broken ConsumerIdKey comparator implementation

["Tim Bain (JIRA)" <ji...@apache.org>]

Tim Bain created AMQ-5474:
-----------------------------

             Summary: Broken ConsumerIdKey comparator implementation
                 Key: AMQ-5474
                 URL: https://issues.apache.org/jira/browse/AMQ-5474
             Project: ActiveMQ
          Issue Type: Bug
          Components: Broker
    Affects Versions: 5.9.0
            Reporter: Tim Bain


One of the changes Gary made under the second batch of changes for AMQ-2327 (https://git-wip-us.apache.org/repos/asf?p=activemq.git;a=commit;h=6c5732bc) involved creating a comparator for AdvisoryBroker.ConsumerIdKey.  This comparator is broken in one way, and inconsistent with ConsumerIdKey.equals() and .hashcode() in two others.  I'm still using 5.8.0 so I can't say whether these problems cause the fix for AMQ-2327 to not work in all cases, but that would be my concern.

Most significantly, if a and b have the same delegate but a's creationTime is before b's, then comparator.compareTo(a, b) == -1 but comparator.compareTo(b, a) == 0.  This is flat-out broken, and will probably cause incorrect sorting in the ConcurrentSkipListMap that was put in place to fix AMQ-2327.

Next, if the creationTimes are equal, the delegates are compared.  But this comparison is done via object equality (==) while ConsumerIdKey.equals() calls the delegate's equals() method.  Presumably only one of these approaches is the right one and it should be used both places; I'm guessing equals() is the way to go, though I could be wrong.

Finally, the comparator is not consistent with equals because there are objects for which a.equals(b) but comparator.compareTo(a, b) != 0.  This might be OK, but I'd encourage another look at both the comparator and ConsumerIdKey.equals() to make sure that that's really the intent.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)