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