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

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(a)jboss.org > Telephone: +44 7786 702 706 > MSN: manik(a)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(a)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(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/jbosscache-dev