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)