On Mon, 2005-11-14 at 19:42 -0500, Sandy McArthur wrote:
> On 11/14/05, robert burrell donkin <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
