Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoadingCache hits downstream multiple time in case of TTL expiry #6180

Closed
swayamraina opened this issue Sep 20, 2022 · 4 comments
Closed

LoadingCache hits downstream multiple time in case of TTL expiry #6180

swayamraina opened this issue Sep 20, 2022 · 4 comments
Labels
P3 package=cache type=enhancement Make an existing feature better

Comments

@swayamraina
Copy link

Hi,

We are using LoadingCache for our use-case where Key and Value are complex objects

private final LoadingCache<KeyObject, ValueObject> loadingCache;

In our service implementation, we try to fetch multiple keys in a go from the cache object

Map<KeyObject, ValueObject> keyZoneSupplyLineCapacityMap = this.loadingCache.getAll(keys);

However in case a Key has expired, Guava (CacheLoader<KeyObject, ValueObject>) immediately calls the downstream to get the fresh value of the object corresponding to the key using reload(KeyObject key, ValueObject oldValue) and this results in multiple downstream calls in case when there are multiple keys in request that have TTL expired.

Since This is a "get multiple keys" call to guava, it makes more sense for LoadingCache to accumulate all keys that have expired and make a single call to downstream to get the updated values, This way it can now merge these fresh values with values for keys that are still not expired.

For implementation, we can use loadAll(Iterable<? extends Key> keys) of the CacheLoader to get values for expired keys.

This new implementation can help reduce service network calls opposite of what we see now where it calls asynchronously for every key.

@ben-manes
Copy link
Contributor

ben-manes commented Sep 20, 2022

I've looked into this previously for #1975 / ben-manes/caffeine#7.

A reason why it hasn't moved forward is that while it makes sense for getAll, that is less often used and it does not optimize for individual reloads triggered by concurrent get calls. Both cases can instead be handled by coalescing individual reload calls for a bulk operation, which the caches handle naturally as the api returns a future. There is a user contributed example (aka not officially supported), as this capability is somewhat more general purpose than only cache-specific and there has not been enough feedback to know the features users might want (batch size, delay period; max in-flight (api quota), etc?). A dedicated library (like how Failsafe is for retry policies) could then help in non-cache usages, like coalescing external api calls or supporting the inverse, an intelligent write-behind. I'd be interested in any thoughts you have since it hasn't been clear what the direction should be, so I have left the issues open but leaned towards closing as out of scope.

@swayamraina
Copy link
Author

like coalescing external api calls or supporting the inverse, an intelligent write-behind.

yeah so by coalescing, I meant was it will do something like this,

List<String> expiredKeys = new ArrayList<>()
for (String key : keys) {
  value = cacheInstance.get(key)
  // TTL expired
  if (value == null) {
     expiredKeys.add(key);
  } else {
    // add to response
    cacheResponse.put(key, value)
  }
  // get updated values for all expired keys
  newValues = loadAll(expiredKeys)
  // merge 
  merge(cacheResponse, newValues)
}

Moreover this works beautifully even if user wants to fetch a single key from cache instead of using current logic of,

for (K key : keys) {
      V value = get(key);    <---- fetch and then load individually
      if (!result.containsKey(key)) {
        result.put(key, value);
        if (value == null) {
          misses++;
          keysToLoad.add(key);
        } else {
          hits++;
        }
      }
    }

caches handle naturally as the api returns a future

So this is good to prevent latency but downstream takes a hit in terms of network bandwidth and throughput (even though it provided bulk API for optimising this use case)

there has not been enough feedback to know the features users might want

Not sure if users moved to their own implementation or any other open-source because of this 😓
Is there a possibility we implement this as a new API for people to use and get active feedback via issues?
PS: willing to contribute to this because of own project need

@ben-manes
Copy link
Contributor

caches handle naturally as the api returns a future

So this is good to prevent latency but downstream takes a hit in terms of network bandwidth and throughput (even though it provided bulk API for optimising this use case)

The reload(key, oldValue) returns a future, but the load operation does not need to be singular. As demonstrated in the above linked example, the first reload could trigger a coalescing period (e.g. 10ms) and submit the work items into a queue. When the period expires, a scheduled thread could then perform all of the pending reloads in a single bulk operation and complete the futures with the results. This adds latency due to the delay to gather work and reduces the downstream hit. This is what you are asking for, and this functionality should be viable for Guava's ListenableFuture rather than Caffeine's CompletableFuture.

Is there a possibility we implement this as a new API for people to use and get active feedback via issues?

The caches already provide the needed API hooks for users to implement it themselves. However it seems to be a bit out of scope for the caches to provide this utility themselves. The analogy is that retries and backoff on a failure are also reasonable needs in practice, but it does not make sense for this resilience logic to be specialized by a caching library for its loads rather than be offered by a generalized library (e.g. failsafe).

PS: willing to contribute to this because of own project need

The above example might be a good starting point for reviewing how it could work, even if you prefer to code it anew. I do think it would be beneficial to have a general purpose library for these cases, but also that is decoupled and independent from these caching projects. I think it is more reasonable to scope our issues to providing the api that is needed for hooking in this functionality.

@amalloy amalloy added type=enhancement Make an existing feature better package=cache P3 labels Sep 30, 2022
@ben-manes
Copy link
Contributor

I think this should be closed and users directed to use their favorite Reactive Streams library. See this example using Resctor, which could trivially be adjusted to complete Guava’s ListenableFuture.

@eamonnmcmanus eamonnmcmanus closed this as not planned Won't fix, can't repro, duplicate, stale Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=cache type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

4 participants