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][dbcp] Ongoing saga of setFactory
Date Thu, 16 Jun 2011 17:39:09 GMT
On 6/16/11 10:19 AM, Mark Thomas wrote:
> On 16/06/2011 17:32, sebb wrote:
>> On 16 June 2011 17:25, Phil Steitz <phil.steitz@gmail.com> wrote:
>>> Yesterday I fixed some [dbcp] "problems" caused by the new [pool]
>>> requirement that setFactory can only be called once.  The quotes are
>>> because most of the problems were redundant calls to setFactory.
>>> The reason that we left setFactory in [pool] is that [dbcp]'s
>>> connection factory constructors call setFactory on the pool passed
>>> to them.  It is an easy "mistake" to create a pool, then create a
>>> connection factory and then do pool.setFactory(factory).  I
>>> eliminated all of these usages from within [dbcp], but I bet a fair
>>> amount of user code will similarly blow up when people upgrade.
>> AIUI, the upgrade does require code to be changed because of the package rename?
> It does, but that is just a search and replace and compilation will fail
> until it is fixed.
>> If so, can we not drop the setFactory method and provide it as a
>> constructor parameter?
> I don't think so. We have been around this buoy already.
>
> To quote Phil's summary:
> <quote>
> I think it is more natural to have
> pool = new GKOP(factory,...) have factory.setPool(this) as a side
> effect than
> factory = new KPOF(...,pool...) have pool.setFactory(this) as a side
> effect
> </quote>
>
> I disagree with Phil on this. Since a pool only has one factory whereas
> a factory may support multiple pools,

I had not thought about it that way before.  What is humorous is
that that logic makes it questionable to pass the pool to the
factory constructor (or at least just one pool).  In the case of
[dbcp]'s PoolableConnectionFactory,  it is hard-wired 1-1 to the
pool because it needs to provide the PoolableConnections that it
sources a reference to their owning pool.  I guess a generalized PCF
could source objects for multiple pools, in which case the
constructor could take a collection of pools.  I now see your
point.  In any case, from my perspective, the requirement to be able
to set the factory is understood and I am fine with keeping the code
as it is, possibly with the small patch below.

Phil


>  I think it is wrong for for "new
> GKOP(factory,...)" to have "factory.setPool(this)" as a side-effect. I
> think "new KPOF(...,pool...)" having "pool.setFactory(this)" as a side
> effect is the more natural choice (as per the current code).
>
>>>  I hate to keep backsliding here, but maybe we should to this in GOP
>>> setFactory:
>>>
>>>             synchronized (factoryLock) {
>>>                 if (this.factory == null) {
>>>                     this.factory = factory;
>>> -                } else {
>>> +                } else if (this.factory != factory) {
>>>                     throw new IllegalStateException("Factory
>>> already set");
>>>                 }
>>>             }
> I'd be OK with that. If we sort out logging we can log it at INFO as
> unnecessary.
>
> Mark
>
>
>
> ---------------------------------------------------------------------
> 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