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