[pool] roadmap

[robert burrell donkin <ro...@blueyonder.co.uk>]

thanks to sandy's hard work, looks like pool's starting to gain some
momentum. this seems like a good time to start a thread about the
future. 

trunk is now versioned 2.0-dev and this seems appropriate since it seems
that the next release of this code will contain quite a few changes. 

however, the 1.x codebase contains a number of fixes to the
synchonization which deserve releasing for those on the older codebase.

(unless anyone else with karma wants to jump in here as a volunteer) i'm
willing to act as release manager for this release. however, i would
need help with this release so i need to gauge the level of support
amongst the wider development community. 

any volunteers willing to help with a 1.3 release?

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[robert burrell donkin <ro...@blueyonder.co.uk>]

On Fri, 2005-11-11 at 00:25 +0000, Stephen Colebourne wrote:
> robert burrell donkin wrote:
> > (unless anyone else with karma wants to jump in here as a volunteer) i'm
> > willing to act as release manager for this release. however, i would
> > need help with this release so i need to gauge the level of support
> > amongst the wider development community. 
> > 
> > any volunteers willing to help with a 1.3 release?
> 
> I am willing to perform my usual release checking.

cool: you do a great job :)

> I MAY be able to get a beta jar run in a load testing environment, 
> however it would be testing [pool] as a tiny component in an enterprise 
> app, so how valuable the test is would be debatable.

you're probably best placed to make that call

> I also think that contacting Spring may be a good idea, as I believe 
> they recommend [pool].

good plans. anyone volunteers?

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[Stephen Colebourne <sc...@btopenworld.com>]

robert burrell donkin wrote:
> (unless anyone else with karma wants to jump in here as a volunteer) i'm
> willing to act as release manager for this release. however, i would
> need help with this release so i need to gauge the level of support
> amongst the wider development community. 
> 
> any volunteers willing to help with a 1.3 release?

I am willing to perform my usual release checking.

I MAY be able to get a beta jar run in a load testing environment, 
however it would be testing [pool] as a tiny component in an enterprise 
app, so how valuable the test is would be debatable.

I also think that contacting Spring may be a good idea, as I believe 
they recommend [pool].

Stephen

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[robert burrell donkin <ro...@blueyonder.co.uk>]

On Mon, 2005-11-14 at 19:42 -0500, Sandy McArthur wrote:
> On 11/14/05, robert burrell donkin <ro...@blueyonder.co.uk> wrote:
> > On Fri, 2005-11-11 at 02:32 -0500, Sandy McArthur wrote:
> > > * The javadacs for [Keyed]ObjectPool.borrowObject should list
> > > NoSuchElementException as one of the thrown exceptions. Techically a
> > > NSEE is included with "throws Exception" but listing NSEE would help
> > > clients better deal with exceptions. If you are using a pool that
> > > limits the number of active objects then a NSEE isn't that big of a
> > > deal relative to any other exception which would indicate a truely
> > > unexpected problem. Since NoSuchElementException extends
> > > RuntimeException adding this wouldn't break API compatability.
> >
> > +1
> >
> > IMHO throwing exception is reasonable but really the API needs to be
> > helping the client code understand the exceptions thrown. (personally
> > speaking i prefer checked exceptions but they come with their own
> > issues.)
> 
> I'm not suggesting the throws Exception is removed, just that
> NoSuchElementException is added to the documentation.

thought you meant that (sorry for the confusion - just stating a general
opinion)

> > > * [Keyed]PoolableObjectFactory in the package description asserts that
> > > activateObject will be invoked on every object returned from the pool.
> > > The method description asserts that activeObject will be used to
> > > "reinitialize an instance to be returned by the pool." What is
> > > ambiguous is if activateObject is used to new created objects from
> > > makeObject.
> > >
> > > It's my opinion that activateObject should only be used on existing
> > > objects that are being returned from the internal idle object pool.
> > > This behavior has the advantage that it's possibly more efficient. In
> > > my experience newly made objects are almost always in the same state
> > > that an activated object would be in.
> >
> > +1
> >
> > IMHO there will be some use cases where calling activateObject on the
> > constructed object is more elegant. however, there seems little point in
> > demanding this behaviour generally. a decorating factory calling
> > activateObject on the product would allow any 1.x factories which
> > require this to be adapted.
> 
> If a PoolableObjectFactory implementation needs to call activateObject
> on newly created objects there is no reason it cannot make that call
> from within the makeObject method.

yep

my reason for proposing a simple decorating factory implementation would
be to give a easy upgrade path for those effected by this change. but
i'm +1 about the change you propose.

> > > * The [Keyed]PoolableObjectFactory getNumIdle and getNumActive methods
> > > currently throw an UnsupportedOperationException if they aren't
> > > supported. I kinda think it would be easier on clients if they simply
> > > reuturned a negative value when those operations are not supported. As
> > > it is now if you have code similar to:
> > > if (pool.getNumActive() > 15) { log.message("borrowed more than expected"); }
> > > you really need to wrap it in a try/catch to be safe which I find a
> > > bit silly. Since UnsupportedOperationException is a RuntimeException
> > > removing this doesn't break API compatability.
> >
> > this is a question of style and i've heard both sides of this argument.
> > however, i do suspect that the exception handling for pool is rather an
> > arcane art and over complex. there may be an argument for allowing
> > implementations flexibility as to whether they thrown an exception or
> > return a negative value.
> 
> I don't strongly care one way or another. But please do not allow
> both. Pick one.

bit of a tough choice. pool is used quite widely
(http://gump.zones.apache.org/gump/test/jakarta-commons/commons-pool/details.html) so preserving binary compatibility would be a nice-to-have but the exceptions make things difficult. 

opinions?

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[Sandy McArthur <sa...@gmail.com>]

On 11/14/05, robert burrell donkin <ro...@blueyonder.co.uk> wrote:
> On Fri, 2005-11-11 at 02:32 -0500, Sandy McArthur wrote:
> > * The javadacs for [Keyed]ObjectPool.borrowObject should list
> > NoSuchElementException as one of the thrown exceptions. Techically a
> > NSEE is included with "throws Exception" but listing NSEE would help
> > clients better deal with exceptions. If you are using a pool that
> > limits the number of active objects then a NSEE isn't that big of a
> > deal relative to any other exception which would indicate a truely
> > unexpected problem. Since NoSuchElementException extends
> > RuntimeException adding this wouldn't break API compatability.
>
> +1
>
> IMHO throwing exception is reasonable but really the API needs to be
> helping the client code understand the exceptions thrown. (personally
> speaking i prefer checked exceptions but they come with their own
> issues.)

I'm not suggesting the throws Exception is removed, just that
NoSuchElementException is added to the documentation.

> > * [Keyed]PoolableObjectFactory in the package description asserts that
> > activateObject will be invoked on every object returned from the pool.
> > The method description asserts that activeObject will be used to
> > "reinitialize an instance to be returned by the pool." What is
> > ambiguous is if activateObject is used to new created objects from
> > makeObject.
> >
> > It's my opinion that activateObject should only be used on existing
> > objects that are being returned from the internal idle object pool.
> > This behavior has the advantage that it's possibly more efficient. In
> > my experience newly made objects are almost always in the same state
> > that an activated object would be in.
>
> +1
>
> IMHO there will be some use cases where calling activateObject on the
> constructed object is more elegant. however, there seems little point in
> demanding this behaviour generally. a decorating factory calling
> activateObject on the product would allow any 1.x factories which
> require this to be adapted.

If a PoolableObjectFactory implementation needs to call activateObject
on newly created objects there is no reason it cannot make that call
from within the makeObject method.

> > * The [Keyed]PoolableObjectFactory getNumIdle and getNumActive methods
> > currently throw an UnsupportedOperationException if they aren't
> > supported. I kinda think it would be easier on clients if they simply
> > reuturned a negative value when those operations are not supported. As
> > it is now if you have code similar to:
> > if (pool.getNumActive() > 15) { log.message("borrowed more than expected"); }
> > you really need to wrap it in a try/catch to be safe which I find a
> > bit silly. Since UnsupportedOperationException is a RuntimeException
> > removing this doesn't break API compatability.
>
> this is a question of style and i've heard both sides of this argument.
> however, i do suspect that the exception handling for pool is rather an
> arcane art and over complex. there may be an argument for allowing
> implementations flexibility as to whether they thrown an exception or
> return a negative value.

I don't strongly care one way or another. But please do not allow
both. Pick one.

--
Sandy McArthur

"He who dares not offend cannot be honest."
- Thomas Paine

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[robert burrell donkin <ro...@blueyonder.co.uk>]

On Fri, 2005-11-11 at 02:32 -0500, Sandy McArthur wrote:
> On 11/10/05, robert burrell donkin <ro...@blueyonder.co.uk> wrote:
> > thanks to sandy's hard work, looks like pool's starting to gain some
> > momentum. this seems like a good time to start a thread about the
> > future.
> 
> Here is my Pool wish list. Since these could change the behavior of
> existing code (but not source code compatability) they should probably
> be held off until Pool 2.0:

it should be not so much a case of holding off as making sure they go
into trunk (rather than the release branch). 

> * Clarify contracts described in the javadocs, specifically:

it's important that any changes are highlighted in the documentation

> * I think an [Keyed]ObjectPool.borrowObject should only return an
> object that is: 1. newly created via PoolableObjectFactory.makeObject
> or 2. previously idle and has passed through
> PoolableObjectFactory.activateObject and
> PoolableObjectFactory.validatedObject or 3. throw an exception.

sounds reasonable but can anyone think of any missing alternative use
cases?  

> * The javadacs for [Keyed]ObjectPool.borrowObject should state by
> contract that borrowed objects must be returned to the pool via
> returnObject OR invalidateObject. Currently it only lists
> returnObject.

+1

any objections?

> * The javadacs for [Keyed]ObjectPool.borrowObject should list
> NoSuchElementException as one of the thrown exceptions. Techically a
> NSEE is included with "throws Exception" but listing NSEE would help
> clients better deal with exceptions. If you are using a pool that
> limits the number of active objects then a NSEE isn't that big of a
> deal relative to any other exception which would indicate a truely
> unexpected problem. Since NoSuchElementException extends
> RuntimeException adding this wouldn't break API compatability.

+1

IMHO throwing exception is reasonable but really the API needs to be
helping the client code understand the exceptions thrown. (personally
speaking i prefer checked exceptions but they come with their own
issues.)

> * [Keyed]PoolableObjectFactory in the package description asserts that
> activateObject will be invoked on every object returned from the pool.
> The method description asserts that activeObject will be used to
> "reinitialize an instance to be returned by the pool." What is
> ambiguous is if activateObject is used to new created objects from
> makeObject.
> 
> It's my opinion that activateObject should only be used on existing
> objects that are being returned from the internal idle object pool.
> This behavior has the advantage that it's possibly more efficient. In
> my experience newly made objects are almost always in the same state
> that an activated object would be in.

+1

IMHO there will be some use cases where calling activateObject on the
constructed object is more elegant. however, there seems little point in
demanding this behaviour generally. a decorating factory calling
activateObject on the product would allow any 1.x factories which
require this to be adapted. 

> Also, It's my opinion if activateObject throws an exception than that
> idle object should be discarded and another should be borrowed from
> the pool. If this fails until there are no more idle objects and newly
> made object is needed it is much easier to write code that only lets
> exceptions from makeObject propagate out ObjectPool.borrowObject.

+1

of course Error's should be propagated. 

> * [Keyed]PoolableObjectFactory in the package description asserts that
> validateObject will only be invoked on an "activated" object. The
> method description says that "Ensures that the instance is safe to be
> returned by the pool. Returns false if this object should be
> destroyed." I pretty much agree with both of those statements.
> 
> It's my opinion that validateObject should only be invoked on: 1.
> previously idle objects that have been activated and are about to be
> borrowed from the pool or 2. optionally by returnObject before
> passivateObject is called or 3. by the idle object evictor but not
> before the evictor has called activateObject and just before the
> evictor then calls passivateObject or destroyObject.

+1

> * It's not clearly stated what is supposed to happen to other methods
> after close has been called on a pool. The existing code throws an
> IllegalStateException when any other method on the pool is invoked.
> Personally, I think only borrowObject should throw an
> IllegalStateException and other methods such as returnObject or
> invalidateObject should just silently fail. My reasoning for this is
> say you have a threaded app with a number of currently borrowed
> objects. You shut the app down which calls the close method. As it is
> now, any worker threads that try to return borrowed objects will
> encounter an IllegalStateException. I think less harm would be done if
> those methods failed silently.

+1

the expected behaviour on close is definitely important for users of
pool and is missing from the javadocs. it needs to be added. 

(the only use for those IllegalStateException's would be as an indirect
way to work out whether the pool is closed but that's a hack and
shouldn't be actively supported.

> * The [Keyed]PoolableObjectFactory getNumIdle and getNumActive methods
> currently throw an UnsupportedOperationException if they aren't
> supported. I kinda think it would be easier on clients if they simply
> reuturned a negative value when those operations are not supported. As
> it is now if you have code similar to:
> if (pool.getNumActive() > 15) { log.message("borrowed more than expected"); }
> you really need to wrap it in a try/catch to be safe which I find a
> bit silly. Since UnsupportedOperationException is a RuntimeException
> removing this doesn't break API compatability.

this is a question of style and i've heard both sides of this argument.
however, i do suspect that the exception handling for pool is rather an
arcane art and over complex. there may be an argument for allowing
implementations flexibility as to whether they thrown an exception or
return a negative value. 

- robert


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [pool] roadmap

[Sandy McArthur <sa...@gmail.com>]

On 11/10/05, robert burrell donkin <ro...@blueyonder.co.uk> wrote:
> thanks to sandy's hard work, looks like pool's starting to gain some
> momentum. this seems like a good time to start a thread about the
> future.

Here is my Pool wish list. Since these could change the behavior of
existing code (but not source code compatability) they should probably
be held off until Pool 2.0:

* Clarify contracts described in the javadocs, specifically:

* I think an [Keyed]ObjectPool.borrowObject should only return an
object that is: 1. newly created via PoolableObjectFactory.makeObject
or 2. previously idle and has passed through
PoolableObjectFactory.activateObject and
PoolableObjectFactory.validatedObject or 3. throw an exception.

* The javadacs for [Keyed]ObjectPool.borrowObject should state by
contract that borrowed objects must be returned to the pool via
returnObject OR invalidateObject. Currently it only lists
returnObject.

* The javadacs for [Keyed]ObjectPool.borrowObject should list
NoSuchElementException as one of the thrown exceptions. Techically a
NSEE is included with "throws Exception" but listing NSEE would help
clients better deal with exceptions. If you are using a pool that
limits the number of active objects then a NSEE isn't that big of a
deal relative to any other exception which would indicate a truely
unexpected problem. Since NoSuchElementException extends
RuntimeException adding this wouldn't break API compatability.

* [Keyed]PoolableObjectFactory in the package description asserts that
activateObject will be invoked on every object returned from the pool.
The method description asserts that activeObject will be used to
"reinitialize an instance to be returned by the pool." What is
ambiguous is if activateObject is used to new created objects from
makeObject.

It's my opinion that activateObject should only be used on existing
objects that are being returned from the internal idle object pool.
This behavior has the advantage that it's possibly more efficient. In
my experience newly made objects are almost always in the same state
that an activated object would be in.

Also, It's my opinion if activateObject throws an exception than that
idle object should be discarded and another should be borrowed from
the pool. If this fails until there are no more idle objects and newly
made object is needed it is much easier to write code that only lets
exceptions from makeObject propagate out ObjectPool.borrowObject.

* [Keyed]PoolableObjectFactory in the package description asserts that
validateObject will only be invoked on an "activated" object. The
method description says that "Ensures that the instance is safe to be
returned by the pool. Returns false if this object should be
destroyed." I pretty much agree with both of those statements.

It's my opinion that validateObject should only be invoked on: 1.
previously idle objects that have been activated and are about to be
borrowed from the pool or 2. optionally by returnObject before
passivateObject is called or 3. by the idle object evictor but not
before the evictor has called activateObject and just before the
evictor then calls passivateObject or destroyObject.

* It's not clearly stated what is supposed to happen to other methods
after close has been called on a pool. The existing code throws an
IllegalStateException when any other method on the pool is invoked.
Personally, I think only borrowObject should throw an
IllegalStateException and other methods such as returnObject or
invalidateObject should just silently fail. My reasoning for this is
say you have a threaded app with a number of currently borrowed
objects. You shut the app down which calls the close method. As it is
now, any worker threads that try to return borrowed objects will
encounter an IllegalStateException. I think less harm would be done if
those methods failed silently.

* The [Keyed]PoolableObjectFactory getNumIdle and getNumActive methods
currently throw an UnsupportedOperationException if they aren't
supported. I kinda think it would be easier on clients if they simply
reuturned a negative value when those operations are not supported. As
it is now if you have code similar to:
if (pool.getNumActive() > 15) { log.message("borrowed more than expected"); }
you really need to wrap it in a try/catch to be safe which I find a
bit silly. Since UnsupportedOperationException is a RuntimeException
removing this doesn't break API compatability.

--
Sandy McArthur

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org