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] POOL-272: per-key numTestsPerEvictionRun
Date Mon, 15 Sep 2014 17:33:18 GMT
On 9/15/14 9:02 AM, Michael Berman wrote:
> I'm fine with pushing it off to 2.4, since it doesn't seem like we're
> converging on an obviously preferred implementation.
>
> That said, I'm not too worried about maintaining the per-key iterator, even
> though the key list is dynamic. The eviction process already maintains its
> own list of keys, which is maintained in parallel with the list of keys in
> pool. It seems like switching from a list<key> to a map<key,iterator> would
> be straightforward, and we could continue maintaining it in exactly the
> same places the existing list is managed.

You are probably right about that.  It is just a little tricky given
the locking stuff.  Patches welcome!

Phil
>
> On Mon, Sep 15, 2014 at 11:56 AM, Phil Steitz <phil.steitz@gmail.com> wrote:
>
>> On 9/11/14 11:39 AM, Phil Steitz wrote:
>>> On 9/11/14 8:55 AM, Phil Steitz wrote:
>>>> On 9/9/14 11:38 AM, Michael Berman wrote:
>>>>> Great, thanks! I have all the new config property boilerplate in place
>> and
>>>>> I understand how it should work, but I'm debating the best way to
>> implement
>>>>> the per-key queues. It would be nice if the test iteration for each key
>>>>> resumed in the same place the next time the cycle hits that key, but
>> this
>>>>> implies keeping around an iterator for every key. Right now, the test
>> cycle
>>>>> attempts to start from where it left off within the key, although it
>> gives
>>>>> up and moves onto the next key pretty aggressively if anything goes
>> wrong,
>>>>> and then starts back at the beginning the next time it comes back
>> around.
>>>>> Do you think that behavior should be preserved?
>>>> I see the problem and I suspect current behavior won't work for what
>>>> you or anyone else wanting this feature would like.  Behavior would
>>>> be OK for the key where the evictor left off, but visits would then
>>>> revert to the same initial instances per key later on.  I can't
>>>> think of a reliable way around this other than maintaining iterators
>>>> for each key, as you suggest.
>>> Thinking about this some more, I am not sure the per key iterator
>>> idea is so hot.  This will be very tricky to maintain given that the
>>> key list itself is dynamic.  Other options would be to select the
>>> instances to hit per key randomly somehow or just keep track of
>>> "offsets" per key.  The latter would not be precise or accurate
>>> given pool membership changes, but would keep the evictor from
>>> getting stuck hitting the beginning of the dequeue all of the time.
>>> Random "offsets" might be a decent strategy for most practical use
>>> cases and would be easy to implement.
>> I guess above is not appealing as it does not guarantee sequential
>> order (or ever) evictor visits.  I will summarize above to the
>> ticket and push this issue out to 2.4 if thats OK.
>>
>> Phil
>>> Phil
>>>> Phil
>>>>> As far as maintaining iterators for each key goes, we could convert
>>>>> poolKeyList, which is only read by the evictor process now into a
>>>>> Map<K,Optional<Iterator<T>>> or something to track
our place iterating
>>>>> through the Deque for keys we've already seen (and just insert None
>> when a
>>>>> new key comes into existence, when we finish an iteration, or if we
>> bail on
>>>>> an iteration after coming across a borrowed object). Does that sound
>>>>> reasonable to you?
>>>>>
>>>>> On Sat, Sep 6, 2014 at 5:22 PM, Phil Steitz <phil.steitz@gmail.com>
>> wrote:
>>>>>> On 9/5/14 8:44 AM, Michael Berman wrote:
>>>>>>> I was proposing having both properties be maxes, not mins; whichever
>> we
>>>>>> hit
>>>>>>> first is the limit.
>>>>>> Sorry, I misunderstood.  That sounds fine to me.  I would be happy
>>>>>> to work with you on a patch implementing this.  If you want to take
>>>>>> a stab at it, just attach the patch (including unit test) to the
>>>>>> ticket.
>>>>>>
>>>>>> Phil
>>>>>>>  I agree that not having a cap on the total number of
>>>>>>> tests performed is dangerous (I wouldn't really set it to max
int;
>> just
>>>>>>> something pretty high). However, there is some precedent for
>> unbounded
>>>>>>> tests: currently, if you set the limit to a negative, the number
of
>> tests
>>>>>>> scales with the number of values. This doesn't seem fundamentally
>> more
>>>>>>> dangerous than scaling with the number of keys.
>>>>>>>
>>>>>>> In some situations, though, it's true that it's important to
be able
>> to
>>>>>> cap
>>>>>>> the total number of tests. By the same logic, I may also not
want an
>>>>>>> unbounded number of tests to be able to occur for each key, which
>> makes
>>>>>> the
>>>>>>> "there will always be numTests performed regardless of the perKey
>>>>>> setting"
>>>>>>> option sound scary. Maybe there should be another switch for
whether
>> we
>>>>>>> want the min or the max?
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Sep 5, 2014 at 11:28 AM, Phil Steitz <phil.steitz@gmail.com>
>>>>>> wrote:
>>>>>>>> On 9/3/14 8:53 AM, Michael Berman wrote:
>>>>>>>>> Thanks for the feedback Phil!
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> One way to do this would be to have the current property
be a cap
>> on
>>>>>>>>>> the total number of tests performed, but the new
one basically
>>>>>>>>>> control the number done per key.  So if, e.g.
>> numTestsPerEvictionRun
>>>>>>>>>> is 12, numEvictionTestsPerKey (or whatever we decide
to call it)
>> is
>>>>>>>>>> 3,  the evictor does 3 tests for each key and then
moves on to the
>>>>>>>>>> next immediately, up to 12.  I guess if there are
fewer than
>>>>>>>>>> numEvictionTestsPerKey, the evictor just moves on.
 If the evictor
>>>>>>>>>> gets all the way through before numTestsPerEcvictionRun
it could
>>>>>>>>>> wrap around or just stop.  Would something like that
work for you?
>>>>>>>>>> Anyone have any better ideas?
>>>>>>>>> "No more than numEvictionTestsPerKey per key and no more
than
>>>>>>>>> numTestsPerEvictionRun total" would definitely address
my use case
>> (in
>>>>>> my
>>>>>>>>> case, I'll just set numTestsPerEvictionRun to Int.MAX
or
>> something),
>>>>>> but
>>>>>>>> I
>>>>>>>>> do wonder if that's what everyone would want. I could
imagine
>> expecting
>>>>>>>> the
>>>>>>>>> behavior to be "At least numEvictionTestsPerKey per key
and at
>> least
>>>>>>>>> numTestsPerEvictionRun total" (which would be the behavior
if we
>>>>>> wrapped
>>>>>>>>> around instead of just stopping). I suppose it's easy
to make it
>> clear
>>>>>>>>> which behavior we have in documentation, but that presumes
the one
>> I
>>>>>> want
>>>>>>>>> is likely to be universally more useful than the alternative.
>> Think I
>>>>>> can
>>>>>>>>> make that assumption?
>>>>>>>> We would have to rename numTestsPerEvictionRun if we were
to change
>>>>>>>> it to mean a min.  I would personally not like that.  I would
rather
>>>>>>>> leave it defined as is and just have the new property control
when
>>>>>>>> the evictor moves from key to key.  Another option would
be to have
>>>>>>>> the new property override the old one - so if numEvictionTestsPerKey
>>>>>>>> is set (i.e. not the default), then numTestsPerEvictionRun
is
>>>>>>>> ignored and the evictor is controlled completely by the second
one.
>>>>>>>>
>>>>>>>> Personally, I would favor the first of my suggestions above
-
>>>>>>>> numTestsPerEvictionRun determines the total number of tests
>>>>>>>> performed and numEvictionTestsPerKey determines how many
tests are
>>>>>>>> done in each key set before moving to the next key.  Not
having a
>>>>>>>> cap on the total number of tests performed is dangerous,
as there is
>>>>>>>> no bound to the number of keys so just doing numEvictionTestsPerKey
>>>>>>>> for every key each evictor run could result in very long-running
>>>>>>>> evictors.
>>>>>>>>
>>>>>>>> It would be good to get some other opinions on this.  Anyone
have
>>>>>>>> ideas / preferences?
>>>>>>>>
>>>>>>>> Phil
>>>>>>>>
>>>>>>>>
>>>>>>>>> With respect to naming:
>>>>>>>>> numEvictionTestsPerKey is definitely more concise and
readable than
>>>>>>>>> numTestsPerEvictionRunPerKey, but I wonder if it makes
it less
>> obvious
>>>>>>>> that
>>>>>>>>> it's tightly connected to numTestsPerEvictionRun. In
code
>> completion,
>>>>>> the
>>>>>>>>> setters wouldn't appear next to each other, which might
hurt
>>>>>>>>> discoverability. Maybe just referencing each other in
docs is
>> enough to
>>>>>>>>> mitigate that. Any votes one way or the other?
>>>>>>>>>
>> ---------------------------------------------------------------------
>>>>>>>> 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
>>>>>>
>>>>>>
>>
>> ---------------------------------------------------------------------
>> 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