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

[Brian Stansberry]

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