commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: [pool] Pool config vs. factory hierarchies.
Date Sun, 31 Oct 2010 03:55:07 GMT
On 10/30/10 10:55 PM, Gary Gregory wrote:
>> -----Original Message----- From: Phil Steitz
>> [mailto:phil.steitz@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.
>
> 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.

> 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. 
  What I am not sure adds value is keeping the Config instances as 
members of the pool classes.

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.

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.steitz@gmail.com>   wrote:
>>>> On 10/29/10 2:41 PM, James Carman wrote:
>>>>>
>>>>> On Fri, Oct 29, 2010 at 2:24 PM, sebb<sebbaz@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-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail:
>>>>> dev-help@commons.apache.org
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>>
>>>>
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail:
>>>> dev-help@commons.apache.org
>>>>
>>>>
>>>
>>> ---------------------------------------------------------------------
>>>
>>>
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>>
>>
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>
>
> ---------------------------------------------------------------------
>
>
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message