On Oct 31, 2010, at 8:55, "Phil Steitz" <phil.ste...@gmail.com> wrote:
> On 10/30/10 10:55 PM, Gary Gregory wrote: >>> -----Original Message----- From: Phil Steitz >>> [mailto:phil.ste...@gmail.com] Sent: Sunday, October 31, 2010 >>> 06:35 To: Commons Developers List Subject: Re: [pool] Pool >>> config vs. factory hierarchies. >>> >>> On 10/30/10 2:30 PM, Simone Tripodi wrote: >>>> Hi Phil, the benefits of eliminating the member variables in >>>> favor of storing pool config reference are IMHO in therms of >>>> code maintainability and keep it as much simple as possible. >>> >>> Maybe I am being dense here, but I don't quite get that. The >>> pool has properties such as numActive that it needs to maintain >>> as well as the config-related properties. Keeping the Config >>> instance around as a new pool property adds complexity and >>> doesn't really improve maintainability, as far as I can see; >>> but it is quite possible I am missing something. >> >> I do not think anyone is being dense here ;) This conversation is >> revealing to me that what the pool concepts I have in my head >> might be based on incorrect assumptions. >> >> When I initially saw the long list of ivars (13 or so I think) >> that GOP and GKOP had in common, that duplication jumped out to >> me as duplication. This is the initial issue I was trying to >> resolve. It turns out that the 'fix' for that also leaked to the >> config classes. >> >> As this thread goes on, it /sounds like/ that the GOP and GKOP >> are really completely different beasts, which justify what >> /appears/ like duplication. Here, it would help to have ivars and >> methods renamed to the best possible names, which would remove >> the appearance of duplication. > > +1 to look carefully at the full list of member fields and exposed properties > of the GOP and GKOP and decide which ones to rename in GKOP and which ones to > drop or redefine. Who can do this asap so we can see and understand our pile better ? >> >> But I do not buy the completely different beasts argument 100% >> because the GKOP pool feels related to the GOP. > > They are not completely unrelated, but the differences are in the core > methods both of the pools and their associated object factories. >> >> It would be possible for example, that the GOP subclass GKOP as a >> degenerate simple case where the GOP has one pool in a GKOP. That >> seems radical, but it would eliminate a lot of apparent code >> duplication: public class AltGenericObjectPool<T> extends >> GenericKeyedObjectPool<T, T>. That would be weird in the sense >> that APIs like borrowObject() and borrowObject(T) would be >> available but you get the idea. > > I understand the reasoning here, but I don't see it as natural and it would > be hard to implement efficiently (at least I don't see an easy way to do it). > Here again, I could be missing something that would make this easy. > Maybe it's another strategy that could plugged in that would give the pool a keyed vs not behavior. >> Finally, if GOP and GKOP are really separate classes not meant to >> share code, then should go back to ivars instead of a config >> object in each pool? > > I like replacing all of the vars in the ctors with Config instances. +1. I dislike classes with laundry lists of cries. > What I am not sure adds value is keeping the Config instances as members of > the pool classes It might be interesting from the pov of being able to answer succinctly: here is how I was configured. But you could do the same by creating a new config object and answering it. > > I don't like having to maintain similar code in GOP and GKOP and as we think > about how to bring in the jdbc-pool stuff, I would like to see if we can > reduce the amount of similar code in these two classes, or even if there is a > way to somehow combine them efficiently, so I am open to ideas like above. That is my ideal goal : efficient combo. GG > > Phil >> >> Gary >> >>> >>>> >>>> You can see the difference between the current >>>> Stack(Keyed)ObjectPool(Factory) - which are implemented >>>> according your vision - and Generic(Keyed)ObjectPool(Factory) >>>> implementations - that are still implemented with my first >>>> refactory. >>> >>> No, these have the same new complexity (vis a vis the original >>> code) and I am not seeing what the benefit is. Keeping the >>> pool properties as member fields, making the Config objects >>> immutable and just having the ctors copy properties from the >>> immutable Config instances (which are then thrown away) seems >>> simpler and no harder to maintain to me. Again, I must be >>> missing something. >>> >>> I guess at the end of the day, I don't see the "Config" >>> abstraction as adding anything post-construction - i.e,, this >>> abstraction is only useful at pool construction time. From the >>> user perspective, the config properties are no different from >>> any other pool properties, nor are they different internally. >>> Why should we need to write Config.maxActive internally, but >>> just use numActive and other properties directly? This looks >>> to me like added complexity that does not add any value, at >>> least for the pools. It makes more sense to me for the >>> factories to hold a reference to a Config instance. >>> >>> Phil >>> >>>> >>>> >>>> On Sat, Oct 30, 2010 at 6:56 PM, Phil >>>> Steitz<phil.ste...@gmail.com> wrote: >>>>> On 10/29/10 2:41 PM, James Carman wrote: >>>>>> >>>>>> On Fri, Oct 29, 2010 at 2:24 PM, sebb<seb...@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> I had overlooked that aspect ... >>>>>>> >>>>>>> If some changes are more expensive to perform, then the >>>>>>> method might want to determine which items have >>>>>>> changed, rather than just reconfiguring everything. >>>>>>> There may be some changes that don't require a pool >>>>>>> update. >>>>>>> >>>>>> >>>>>> Right now, it appears that they just call that "allocate" >>>>>> method which seems like they kind of "nuke and pave", so >>>>>> I don't think it's that big of a deal. >>>>>> >>>>>>> Factory reconfig probably just needs to update the >>>>>>> stored config variable, which can be volatile. >>>>>>> >>>>>> >>>>>> I'm not familiar with this factory config stuff. I'll >>>>>> have to dig in further. >>>>> >>>>> Sorry to be late on this. Here are the requirements: >>>>> >>>>> 1. Some subset of the config properties (need to decide >>>>> this - should be topic of a different thread) need to be >>>>> *individually* mutable at runtime - e.g., >>>>> setMaxActive(newMaxActive) needs to remain. We have agreed >>>>> at this point that at least maxActive and maxWait need to >>>>> be runtime mutable. >>>>> >>>>> 2. Correct functioning of the pool with the current >>>>> implementation requires that no thread can change maxActive >>>>> while another thread holds the lock on the pool's monitor. >>>>> Just making the properties volatile or protecting them with >>>>> another lock will cause problems. >>>>> >>>>> I am OK keeping the mutable Config instances around, but I >>>>> don't see any real advantage to eliminating the member >>>>> variables storing pool config properties - i.e., my >>>>> preference would be to make the Config instances immutable >>>>> and only used as structs for ctors. >>>>> >>>>> I am +0 on adding a (pool-synchronized) reconfigure(Config) >>>>> to enable multiple properties to be changed atomically. >>>>> >>>>> Phil >>>>> >>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> >>>>>> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>>> For additional commands, e-mail: >>>>>> dev-h...@commons.apache.org >>>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> >>>>> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>>> For additional commands, e-mail: >>>>> dev-h...@commons.apache.org >>>>> >>>>> >>>> >>>> --------------------------------------------------------------------- >>>> >>>> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>>> For additional commands, e-mail: dev-h...@commons.apache.org >>>> >>> >>> >>> --------------------------------------------------------------------- >>> >>> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >> >> >> --------------------------------------------------------------------- >> >> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
