rls: Fix time handling in CachingRlsLbClient

`getMinEvictionTime()` was fixed to make sure only deltas were used for
comparisons (`a < b` is broken; `a - b < 0` is okay). It had also
returned `0` by default, which was meaningless as there is no epoch for
`System.nanoTime()`. LinkedHashLruCache now passes the current time into
a few more functions since the implementations need it and it was
sometimes already available. This made it easier to make some classes
static.
This commit is contained in:
Eric Anderson 2024-03-10 09:08:15 -07:00
parent 056195401f
commit da619e2bde
2 changed files with 15 additions and 22 deletions

View File

@ -528,7 +528,7 @@ final class CachingRlsLbClient {
} }
/** Common cache entry data for {@link RlsAsyncLruCache}. */ /** Common cache entry data for {@link RlsAsyncLruCache}. */
abstract class CacheEntry { abstract static class CacheEntry {
protected final RouteLookupRequest request; protected final RouteLookupRequest request;
@ -538,16 +538,12 @@ final class CachingRlsLbClient {
abstract int getSizeBytes(); abstract int getSizeBytes();
final boolean isExpired() {
return isExpired(ticker.read());
}
abstract boolean isExpired(long now); abstract boolean isExpired(long now);
abstract void cleanup(); abstract void cleanup();
protected long getMinEvictionTime() { protected boolean isOldEnoughToBeEvicted(long now) {
return 0L; return true;
} }
} }
@ -649,8 +645,8 @@ final class CachingRlsLbClient {
} }
@Override @Override
protected long getMinEvictionTime() { protected boolean isOldEnoughToBeEvicted(long now) {
return minEvictionTime; return minEvictionTime - now <= 0;
} }
@Override @Override
@ -678,7 +674,7 @@ final class CachingRlsLbClient {
* Implementation of {@link CacheEntry} contains error. This entry will transition to pending * Implementation of {@link CacheEntry} contains error. This entry will transition to pending
* status when the backoff time is expired. * status when the backoff time is expired.
*/ */
private final class BackoffCacheEntry extends CacheEntry { private static final class BackoffCacheEntry extends CacheEntry {
private final Status status; private final Status status;
private final BackoffPolicy backoffPolicy; private final BackoffPolicy backoffPolicy;
@ -841,7 +837,7 @@ final class CachingRlsLbClient {
@Override @Override
protected boolean isExpired(RouteLookupRequest key, CacheEntry value, long nowNanos) { protected boolean isExpired(RouteLookupRequest key, CacheEntry value, long nowNanos) {
return value.isExpired(); return value.isExpired(nowNanos);
} }
@Override @Override
@ -851,8 +847,8 @@ final class CachingRlsLbClient {
@Override @Override
protected boolean shouldInvalidateEldestEntry( protected boolean shouldInvalidateEldestEntry(
RouteLookupRequest eldestKey, CacheEntry eldestValue) { RouteLookupRequest eldestKey, CacheEntry eldestValue, long now) {
if (eldestValue.getMinEvictionTime() > now()) { if (!eldestValue.isOldEnoughToBeEvicted(now)) {
return false; return false;
} }

View File

@ -85,8 +85,8 @@ abstract class LinkedHashLruCache<K, V> implements LruCache<K, V> {
// first, remove at most 1 expired entry // first, remove at most 1 expired entry
boolean removed = cleanupExpiredEntries(1, ticker.read()); boolean removed = cleanupExpiredEntries(1, ticker.read());
// handles size based eviction if necessary no expired entry // handles size based eviction if necessary no expired entry
boolean shouldRemove = boolean shouldRemove = !removed
!removed && shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value); && shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value, ticker.read());
if (shouldRemove) { if (shouldRemove) {
// remove entry by us to make sure lruIterator and cache is in sync // remove entry by us to make sure lruIterator and cache is in sync
LinkedHashLruCache.this.invalidate(eldest.getKey(), EvictionType.SIZE); LinkedHashLruCache.this.invalidate(eldest.getKey(), EvictionType.SIZE);
@ -102,7 +102,7 @@ abstract class LinkedHashLruCache<K, V> implements LruCache<K, V> {
* that LruCache is access level and the eldest is determined by access pattern. * that LruCache is access level and the eldest is determined by access pattern.
*/ */
@SuppressWarnings("unused") @SuppressWarnings("unused")
protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue) { protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue, long now) {
return true; return true;
} }
@ -236,10 +236,6 @@ abstract class LinkedHashLruCache<K, V> implements LruCache<K, V> {
} }
} }
protected long now() {
return ticker.read();
}
/** /**
* Cleans up cache if needed to fit into max size bytes by * Cleans up cache if needed to fit into max size bytes by
* removing expired entries and removing oldest entries by LRU order. * removing expired entries and removing oldest entries by LRU order.
@ -253,13 +249,14 @@ abstract class LinkedHashLruCache<K, V> implements LruCache<K, V> {
return false; return false;
} }
// cleanup expired entries // cleanup expired entries
cleanupExpiredEntries(now()); long now = ticker.read();
cleanupExpiredEntries(now);
// cleanup eldest entry until new size limit // cleanup eldest entry until new size limit
Iterator<Map.Entry<K, SizedValue>> lruIter = delegate.entrySet().iterator(); Iterator<Map.Entry<K, SizedValue>> lruIter = delegate.entrySet().iterator();
while (lruIter.hasNext() && estimatedMaxSizeBytes < this.estimatedSizeBytes.get()) { while (lruIter.hasNext() && estimatedMaxSizeBytes < this.estimatedSizeBytes.get()) {
Map.Entry<K, SizedValue> entry = lruIter.next(); Map.Entry<K, SizedValue> entry = lruIter.next();
if (!shouldInvalidateEldestEntry(entry.getKey(), entry.getValue().value)) { if (!shouldInvalidateEldestEntry(entry.getKey(), entry.getValue().value, now)) {
break; // Violates some constraint like minimum age so stop our cleanup break; // Violates some constraint like minimum age so stop our cleanup
} }
lruIter.remove(); lruIter.remove();