Return-Path: 
 <dev-return-127072-apmail-commons-dev-archive=commons.apache.org@commons.apache.org>
X-Original-To: apmail-commons-dev-archive@www.apache.org
Delivered-To: apmail-commons-dev-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 11C564C7A
	for <apmail-commons-dev-archive@www.apache.org>;
 Sat, 25 Jun 2011 23:28:54 +0000 (UTC)
Received: (qmail 83455 invoked by uid 500); 25 Jun 2011 23:28:53 -0000
Delivered-To: apmail-commons-dev-archive@commons.apache.org
Received: (qmail 83367 invoked by uid 500); 25 Jun 2011 23:28:52 -0000
Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@commons.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@commons.apache.org>
List-Post: <mailto:dev@commons.apache.org>
List-Id: <dev.commons.apache.org>
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Delivered-To: mailing list dev@commons.apache.org
Received: (qmail 83359 invoked by uid 99); 25 Jun 2011 23:28:52 -0000
Received: from minotaur.apache.org (HELO minotaur.apache.org) (140.211.11.9)
    by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 25 Jun 2011 23:28:52 +0000
Received: from localhost (HELO s2laptop.local) (127.0.0.1)
  (smtp-auth username markt, mechanism plain)
  by minotaur.apache.org (qpsmtpd/0.29) with ESMTP;
 Sat, 25 Jun 2011 23:28:52 +0000
Message-ID: <4E066F32.7000600@apache.org>
Date: Sun, 26 Jun 2011 00:28:50 +0100
From: Mark Thomas <markt@apache.org>
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-GB;
 rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11
MIME-Version: 1.0
To: Commons Developers List <dev@commons.apache.org>
Subject: Re: [pool][dbcp] Ongoing saga of setFactory
References: <4DFA2E66.6020001@gmail.com>
 <BANLkTikyhmNHdvsW5xET37MbNYJzXeZzvw@mail.gmail.com>
 <4DFA3B1B.9030608@apache.org> <4DFA3FBD.30703@gmail.com>
 <8101915826986564492@unknownmsgid> <4DFB0A30.1010708@apache.org>
In-Reply-To: <4DFB0A30.1010708@apache.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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).

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