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

[Brian Stansberry]

These were checked in a few days back.  I didn't close the JIRA 'cause
there were so many failures on cc it was hard for me to guarantee I
didn't cause a problem when I ran the testsuite.  I'd compared my
results to cc and saw no regressions, so I think it was OK, so I'll go
ahead and close 874; we can re-open if the work on nailing down the
remaining failures shows something amiss.

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