commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: [pool][dbcp] Ongoing saga of setFactory
Date Thu, 16 Jun 2011 17:19:23 GMT
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 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


Mime
View raw message