[jbosscache-dev] RE: JBCACHE-874 (WAS: JBCACHE-315 and 1.4.1)

[Manik Surtani]

Yeah, I'm looking into the CC failures today, there does seem to be a  
proliferation of failures (almost 4% at the moment).
--
Manik Surtani

Lead, JBoss Cache
JBoss, a division of Red Hat

Email: manik at jboss.org
Telephone: +44 7786 702 706
MSN: manik at surtani.org
Yahoo/AIM/Skype: maniksurtani



On 8 Dec 2006, at 04:36, Brian Stansberry wrote:

> This change is made on 1.4 and HEAD.
>
> When I looked at it, the effiency gain from using a Set vs a List only
> exists with the locks collection. So that's the only one I changed to
> LinkedHashSet. The methods that add things to the other list fields
> (e.g. modifications) don't do a contains() check to see if what  
> they are
> adding is already there, so they don't have the inefficiency.  
> Presumably
> this means that with normal behavior the same object wouldn't get  
> added
> twice to these lists.  A cursory check showed that to be the case, and
> if it weren't I'd expect we'd have noticeable bugs. So, I decided  
> not to
> change to other fields to LinkedHashSet -- didn't want to introduce
> issues by making a change. At least not in 1.4.
>
> I ran the testsuite and saw no regressions vs cc.  But, there are a  
> lot
> of failures now on cc, so I'm going to wait to close JBCACHE-874 until
> we get some cleaner test runs.
>
> jbosscache-dev-bounces at lists.jboss.org wrote:
>> Manik Surtani wrote:
>>> On 5 Dec 2006, at 19:29, Brian Stansberry wrote:
>>>
>>>> Manik Surtani wrote:
>>>>> Brian Stansberry wrote:
>>>>>> It's not a big deal if JBCACHE-874 doesn't go in 1.4.1.CR1,  
>>>>>> but it
>>>>>> would be nice to get it in so it's out in the wild longer before
>>>>>> the GA.
>>>>>
>>>>> Sorry yes, forgot about this.
>>>>>
>>>>
>>>> If you're delaying 1.4.1.CR1 for this, let me know, because at this
>>>> point I was deferring it a bit pending anything that comes out of
>>>> this thread.
>>>
>>> What would you like me to do?  When do you think you could have 874
>>> in?
>>>
>>
>> Probably by tomorrow. If there's going to be a CR2 I'd prefer
>> you don't delay CR1 for this. Getting things out in the wild
>> is good, the benefit of having this out in CR1 is outweighed
>> by the benefit of getting everything else in CR1 out in the
>> wild sooner.
>>
>>>>
>>>>>>
>>>>>> 1) A bunch of the LinkedList fields in TransactionEntry are
>>>>>> protected. Changing them to LinkedHashSet therefore technically
>>>>>> changes the API. This is an internal class, so I don't think
>>>>>> that's an issue.  Any objections?
>>>>>
>>>>> I'm ok with this, I don't think it will break the
>>>>> OptimisticTransactionEntry subclass.  In fact, is there any reason
>>>>> why this needs to be protected?  Could we make do with stronger
>>>>> access protection than protected?
>>>>>
>>>>
>>>> I don't see any reason why not; this is an internal class. I much
>>>> prefer private fields; hate protected 'cause it's never clear to me
>>>> if the class designer had a strong reason to expose them or just  
>>>> did
>>>> it "in case some subclass wants it some day".  IMHO a protected
>>>> getter/setter with a private field is a much better indication of
>>>> design intent and allows clean subsequent addition of things like
>>>> lazy initialization.
>>>
>>> +1.  And like I said, if we can make do with package-level access  
>>> for
>>> the getter/setter and private for the field, I'd go with that if it
>>> makes sense.
>>>
>>
>> I'll check; we may not even need the getters/setters for this
>> particular case.  I don't think the subclass uses any of the
>> protected fields. And if not, I don't think there is any real
>> "design intent" to expose them.
>> If later on, we find a real need to expose them, then we add them.
>>
>>>>
>>>>>>
>>>>>> 2) A number of TransactionEntry methods return List and currently
>>>>>> hand out a ref to the internal data structure.  The caller of
>>>>>> these methods logically expects a List so I think the API should
>>>>>> remain the same. So, that implies the methods would do something
>>>>>> like "return new ArrayList(internalSet);"
>>>>>>
>>>>>
>>>>> Hmm, I guess so.  Perhaps in 2.0.0, change this to return a
>>>>> Collection?
>>>>>
>>>>
>>>> Maybe. Depends on whether callers are relying on an expected
>>>> ordering. I had a quick look at these methods yesterday.  At least
>>>> one is never invoked and thus can be dropped. For another, at least
>>>> in some cases, Set semantics are fine, so a possibility for  
>>>> 1.4.1 is
>>>> an added method that exposes the internal Set.
>>>
>>> Ok, I'm happy with this.  Like you said, this *is* an internal class
>>> anyway.
>>>
>>
>> Yes. And if there is a path whereby outside code can access
>> this class (I think there is), adding methods doesn't break
>> any compatibility.
>>
>>>>
>>>> Another possibility for 2.0.0 is to return an Iterator over the
>>>> internal field, which would give the same ordering as a List  
>>>> with no
>>>> extra effort.  I expect all the callers already end up iterating
>>>> over the List anyway.
>>>
>>> Is this the case?  What about calling contains() on the List?  I
>>> thought I saw this somewhere ...
>>
>> You did, but the contains() call is done internally to
>> TransactionEntry whenever any of the add methods are called
>> -- in order to ensure Set semantics on the internal lists.
>> Getting rid of this inefficiency is the primary benefit of
>> JBCACHE-874.
>>
>> - Brian
>>
>> _______________________________________________
>> jbosscache-dev mailing list
>> jbosscache-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/jbosscache-dev