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 Sun, 26 Jun 2011 00:05:29 GMT
On 6/25/11 4:28 PM, Mark Thomas wrote:
> On 17/06/2011 09:02, Mark Thomas wrote:
>> On 17/06/2011 00:32, Gary Gregory wrote:
>>> I think 2.0 is the opportunity to do this right. Almost like we were
>>> designing this from scratch.
>>>
>>> Making the factory an invariant of the pool sounds good.
>>>
>>> Otoh If a setFactory method exists it should be implemented fully. The
>>> throw an exception impl is pretty "smelly".
>> I agree that it is smelly. I'm not sure we can change this without a
>> huge amount of work in DBCP.
> I think largely due to the clean-up Phil has already done in DBCP, this
> is now manageable. I have just done this for GOP and will do the same
> for GKOP next (exact timing TBD).

+1 - definitely better.  Will test and review and if I get to if
before you, handle GKOP.   Thanks for doing this.

Phil
> Mark
>
>> Mark
>>
>>> Gary
>>>
>>> On Jun 16, 2011, at 13:39, Phil Steitz <phil.steitz@gmail.com> wrote:
>>>
>>>> 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
>>>>
>>> ---------------------------------------------------------------------
>>> 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