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

[Manik Surtani]

Great - looks ok on CC now so I'll go ahead and close.

Thanks!
--
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 11 Dec 2006, at 20:39, Brian Stansberry wrote:

> 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