On 8/24/12 8:39 AM, Mark Thomas wrote:
> On 24/08/2012 16:22, Phil Steitz wrote:
>> 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.
> Fair enough.
>
>>> - 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).
> Fair enough. Works for me since I was thinking pretty much the same.
>
>>> - 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).
> The patch is missing the AbandonedConfig file which I suspect would have
> some Javadoc that would help here.
Oops! Forgot the svn add there.
>
>>> - 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;
>> }
> You can't differentiate between still allocated (and abandoned) and
> return and re-allocated.
Good catch. Shows the need for some more / better tests. I will
see if I can code some tests that fail with the current patch and
succeed with a correct impl along the lines of what you describe below.
>
>>> - 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?
> The current patch does not perform the following actions in an atomic
> manner:
> - check to see if object is abandoned
> - mark the object as abandoned / invalidate it
>
> Currently these steps are separate. All sorts of things could happen
> between the two steps. With the aim of avoiding the sort of timing bugs
> that gave me a headache with POOLv1, I would much prefer to see a method
> (probably on PooledObject) that checked the last used time and changed
> the state atomically.
Great idea. I was trying to use the PooledObject's monitor and
external logic to do this; but as you point out, I made at least one
mistake. I agree this brings back memories of our fun with latches
in 1.5.x. I will take a crack at a correct impl using the ideas
above and commit.
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
