On Oct 31, 2010, at 9:39, "Gary Gregory" <ggreg...@seagullsoftware.com> wrote:
> 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 $&@! iPhone. That's 'ctors', not '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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
