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

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