issue #72, Exipration stuff #125
Conversation
|
Test PASSed. |
|
|
||
| switch (timeUnit) { | ||
| case MICROSECONDS: | ||
| case NANOSECONDS: { |
There was a problem hiding this comment.
Not only am I not a fan of the restriction (and wasn't in 107 neither), but also why would 1,000,000 TimeUnit.NANOSECONDS not be acceptable and would have to be represented in TimeUnit.MILLISECONDS ?
Long story short, why do we limit this here. Why wouldn't it the Store 's concern to decide how it deals with too small values?
There was a problem hiding this comment.
Yeah I did without thinking too much about it, and obviously I've let 107 infect me.
To be sure, I think you're making two points:
(1) why disallow specified a duration that is effectively equal or greater than 1 ms, even if it expressed in a smaller time unit
(2) Why limit the durations at all, let every Store decide if a duration is too small for it
They're both reasonable. I have hesitation with (2) though. In reality we don't know of a Store that could provide less than millisecond precision (I don't at least). It also seems complicated to decide how to log when a Duration too small is observed (since it happens for most every operation).
We can always remove the limitation later?
I don't feel too strongly about this to be honest. I'm definitely lazy in not wanting to deal with in multiple Store impls though
There was a problem hiding this comment.
Yeah, that's it... but true that! Rather remove the limitation later than the other way around. Again, with 107 in mind, this passes all test!
Now, I still think that point 1 is relevant... and if we convert to "smallest unit" supported (i.e. MILLISECONDS), it would always resolve... including to possibly 0. Anyways we can worry about this later. If you decide to leave it as so, could you just file an issue wrt that?
|
Test PASSed. |
|
Test FAILed. |
|
Test PASSed. |
2 similar comments
|
Test PASSed. |
|
Test PASSed. |
| checkKey(key); | ||
| checkValue(value); | ||
|
|
||
| final AtomicReference<OnHeapValueHolder<V>> returnValue = new AtomicReference<OnHeapValueHolder<V>>(null); |
There was a problem hiding this comment.
This is subjective and mostly about stylistic: but you could make that BiFunction below side-effect free by building the new ValueHolder prior to invoking it. Have the method "close over" it (as well as the now, which I initially didn't like, but since it's used in a Map.compute, it actually lowers the contention on the key, while widening the timing "error", but the former is probably desirable over the latter... so I think it's actually desirable this way).
Anyways, having the method side-effect free is probably more functional... or so they say.
There was a problem hiding this comment.
I considered creating the new ValueHolder outside the function as well. My reasoning for doing it inside was so that the Expiry.getExpiryForCreation() would only ever be called if something was indeed being added. Creating the potential new value holder up front makes the call on the Expiry but that might not end up in the store.
I'm happy to change things to minimize the work inside the function if desired, but I did have my reason for doing it the way it is
There was a problem hiding this comment.
Ah! Yeah... that's a good reason indeed. Should have looked into the newValueHolder method indeed. Given this is only about "style", I'd keep it as you implemented this indeed. Probably "least surprise" to the end-user!
|
Test PASSed. |
timeck
changed the title
|
Test PASSed. |
1 similar comment
|
Test PASSed. |
| // } | ||
| // | ||
| // return new TimeToLiveAndTimeToIdleExpiry(timeToLive, timeToIdle); | ||
| // } |
There was a problem hiding this comment.
Is that a leftover? Or something that might be needed in the future?
There was a problem hiding this comment.
I kinda left it in there commented since it might needed in the future, but given that I usually request others to remove commented code I should do the same
|
Not much to add from me |
|
Before merging this, wait for timeck/ehcache3@c16c51b to be added to it |
present but expired entries
timestamp isn't needed unless the function is actually invoked
Conflicts: core/src/main/java/org/ehcache/spi/cache/Store.java
|
Test FAILed. |
2 similar comments
|
Test FAILed. |
|
Test FAILed. |
|
test this please |
|
Test PASSed. |
|
Test FAILed. |
|
test this please |
|
Test PASSed. |
|
Looks good to me know - not merging just yet to make sure other reviewers get a final look. |
Plenty of questionable things in here:
org.ehcache.expiry.ExpiryExpirygets into a Cache and then onto the Store, as explicit methods on the config objectsTimeSourcegets into OnHeapStore (no idea if I used the service config concept correctly)org.ehcache.internal.store.OnHeapStore.Provider