Mailing-List: contact dev-help@activemq.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@activemq.apache.org
Date: Wed, 3 Dec 2014 20:44:12 +0000 (UTC)
From: "Tim Bain (JIRA)" <jira@apache.org>
To: dev@activemq.apache.org
Message-ID: <JIRA.12759263.1417635176000.71017.1417639452689@Atlassian.JIRA>
In-Reply-To: <JIRA.12759263.1417635176000@Atlassian.JIRA>
References: <JIRA.12759263.1417635176000@Atlassian.JIRA>
 <JIRA.12759263.1417635176633@arcas>
Subject: [jira] [Commented] (AMQ-5474) Broken ConsumerIdKey comparator
 implementation
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


    [ https://issues.apache.org/jira/browse/AMQ-5474?page=3Dcom.atlassian.j=
ira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D142334=
98#comment-14233498 ]=20

Tim Bain commented on AMQ-5474:
-------------------------------

I should have checked to be sure that code was still in 5.10.0 instead of r=
elying solely on the diffs from Gary's commit.  Thanks.

> 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-23=
27 (https://git-wip-us.apache.org/repos/asf?p=3Dactivemq.git;a=3Dcommit;h=
=3D6c5732bc) involved creating a comparator for AdvisoryBroker.ConsumerIdKe=
y.  This comparator is broken in one way, and inconsistent with ConsumerIdK=
ey.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 creationTim=
e is before b's, then comparator.compareTo(a, b) =3D=3D -1 but comparator.c=
ompareTo(b, a) =3D=3D 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 th=
is comparison is done via object equality (=3D=3D) while ConsumerIdKey.equa=
ls() calls the delegate's equals() method.  Presumably only one of these ap=
proaches is the right one and it should be used both places; I'm guessing e=
quals() is the way to go, though I could be wrong.
> Finally, the comparator is not consistent with equals because there are o=
bjects for which a.equals(b) but comparator.compareTo(a, b) !=3D 0.  This m=
ight be OK (consistency with equals isn't required, though it will result i=
n behavior that's consistent but unexpected), but I'd encourage another loo=
k at both the comparator and ConsumerIdKey.equals() to make sure that that'=
s really the intent.  In particular, I'm concerned about whether consumers.=
remove(new ConsumerIdKey(info.getConsumerId())) will work properly if the c=
omparator isn't consistent with equals, since for a SortedMap we're going t=
o be using compareTo() and it doesn't consider two keys equal if they have =
the same delegate but different creationTimes.  (That's the "consistent but=
 unexpected" behavior I referenced earlier.)  So the current behavior *migh=
t* be acceptable on this point, but I'm very skeptical and I'd want a unit =
test that proves that the put(), get(), and remove() operations really work=
 as a developer would expect before accepting that it's valid for this comp=
arator to be inconsistent with equals.


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