Skip to content

Commit

Permalink
rls: Fix time handling in CachingRlsLbClient
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
ejona86 committed Apr 25, 2024
1 parent 0561954 commit da619e2
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 22 deletions.
22 changes: 9 additions & 13 deletions rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ public String toString() {
}

/** Common cache entry data for {@link RlsAsyncLruCache}. */
abstract class CacheEntry {
abstract static class CacheEntry {

protected final RouteLookupRequest request;

Expand All @@ -538,16 +538,12 @@ abstract class CacheEntry {

abstract int getSizeBytes();

final boolean isExpired() {
return isExpired(ticker.read());
}

abstract boolean isExpired(long now);

abstract void cleanup();

protected long getMinEvictionTime() {
return 0L;
protected boolean isOldEnoughToBeEvicted(long now) {
return true;
}
}

Expand Down Expand Up @@ -649,8 +645,8 @@ boolean isStaled(long now) {
}

@Override
protected long getMinEvictionTime() {
return minEvictionTime;
protected boolean isOldEnoughToBeEvicted(long now) {
return minEvictionTime - now <= 0;
}

@Override
Expand Down Expand Up @@ -678,7 +674,7 @@ public String toString() {
* Implementation of {@link CacheEntry} contains error. This entry will transition to pending
* 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 BackoffPolicy backoffPolicy;
Expand Down Expand Up @@ -841,7 +837,7 @@ private static final class RlsAsyncLruCache

@Override
protected boolean isExpired(RouteLookupRequest key, CacheEntry value, long nowNanos) {
return value.isExpired();
return value.isExpired(nowNanos);
}

@Override
Expand All @@ -851,8 +847,8 @@ protected int estimateSizeOf(RouteLookupRequest key, CacheEntry value) {

@Override
protected boolean shouldInvalidateEldestEntry(
RouteLookupRequest eldestKey, CacheEntry eldestValue) {
if (eldestValue.getMinEvictionTime() > now()) {
RouteLookupRequest eldestKey, CacheEntry eldestValue, long now) {
if (!eldestValue.isOldEnoughToBeEvicted(now)) {
return false;
}

Expand Down
15 changes: 6 additions & 9 deletions rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ protected boolean removeEldestEntry(Map.Entry<K, SizedValue> eldest) {
// first, remove at most 1 expired entry
boolean removed = cleanupExpiredEntries(1, ticker.read());
// handles size based eviction if necessary no expired entry
boolean shouldRemove =
!removed && shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value);
boolean shouldRemove = !removed
&& shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value, ticker.read());
if (shouldRemove) {
// remove entry by us to make sure lruIterator and cache is in sync
LinkedHashLruCache.this.invalidate(eldest.getKey(), EvictionType.SIZE);
Expand All @@ -102,7 +102,7 @@ protected boolean removeEldestEntry(Map.Entry<K, SizedValue> eldest) {
* that LruCache is access level and the eldest is determined by access pattern.
*/
@SuppressWarnings("unused")
protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue) {
protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue, long now) {
return true;
}

Expand Down Expand Up @@ -236,10 +236,6 @@ public final List<V> values() {
}
}

protected long now() {
return ticker.read();
}

/**
* Cleans up cache if needed to fit into max size bytes by
* removing expired entries and removing oldest entries by LRU order.
Expand All @@ -253,13 +249,14 @@ protected final boolean fitToLimit() {
return false;
}
// cleanup expired entries
cleanupExpiredEntries(now());
long now = ticker.read();
cleanupExpiredEntries(now);

// cleanup eldest entry until new size limit
Iterator<Map.Entry<K, SizedValue>> lruIter = delegate.entrySet().iterator();
while (lruIter.hasNext() && estimatedMaxSizeBytes < this.estimatedSizeBytes.get()) {
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
}
lruIter.remove();
Expand Down

0 comments on commit da619e2

Please sign in to comment.