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] POOL-229 (move AbandonedObjectPool to pool)
Date Fri, 24 Aug 2012 15:22:26 GMT
On 8/24/12 7:42 AM, Mark Thomas wrote:
> On 22/08/2012 13:51, Phil Steitz wrote:
>> Any feedback on the patch?  Should I go ahead and apply the [pool]
>> patch?
> Sorry for the very late review. Some comments/questions on the pool2
> part (I haven't looked at DBCP):
>
> - Why not add abandoned support to GKOP as well as GOP?

Could definitely be done, but GKOP is tricky because of the dual
parameter set (per key / global) and starvation prevention stuff
already there.  I don't see the need to add it right away, but am OK
with the idea and might eventually get to it.
> - I wasn't convinced at first about separating standard and abandoned
>   config. It is growing on me but I'd be interested in hearing your
>   reasons for this.

I was in exactly the same place.  I actually coded an initial
version combining it all, looked at it and thought it was simpler to
keep them separate.  I do not have strong feelings either way.  I
chose to keep them separate because a) it was a smaller change and
b) I liked the encapsulation of abandoned object cleanup parameters
(and not having to look at them if not using this).
> - What is the reasoning behind the tests:
>   - getNumIdle() < 2
>   - getNumActive() > getMaxTotal() - 3
>   in borrowObject()? It would be good to document these reasons in a
>   comment.
Hmm.  Archaeology ;)
Seriously, I suspect the original reasoning behind this test remains
valid for activation within borrowObject - you don't want to take
the hit of doing an abandoned cleanup if you are flush with
instances.  Note that there is a flag that controls whether
borrowObject does cleanup at all (actually was in the original).
> - I think an explicit state of PooledObjectState.ABANDONED would be
>   clearer / easier to work with (see below).

You are probably right.
> - There are various timing issues/questions in removeAbandoned()
>   - what is the lock on allObjects meant to achieve?
It was intended to ensure that remove was formed using a consistent
snapshot.  Given the way ConcurrentHashMap works, this is not
correct.  So there is no value in the lock.
>   - an object may be returned and re-borrowed after being added to
>     "remove" and before it is invalidated
Right.  That is why this check is there
 if (pooledObject.getState() != PooledObjectState.ALLOCATED) {
                    continue;
}
> - abandonedConfig should be in the config section not the JMX section

Good catch.  Will add that.
>
>
> My instinct is that a checkAbandoned() method on PooledObject that
> changes the state to PooledObjectState.ABANDONED and returns true if it
> does this is likely to be an easier starting point for a thread-safe
> solution rather than separating the timing check and the state change.

Can you explain more what you mean here?

Thanks for the review!

Phil
>
> 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


Mime
View raw message