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

[Manik Surtani]

Brian,

Do you want to check in your fixes for 874 (if you haven't already)?   
Getting close to a clean build on 1.4.x, with a few MUX and PojoCache  
failures remaining.

Cheers,
--
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 11:44, Manik Surtani wrote:

> 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
>
>