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

Fix cache compute if absent for expired entries #26516

Merged
merged 2 commits into from Sep 6, 2017

Conversation

@jasontedor
Copy link
Member

jasontedor commented Sep 6, 2017

When a cache entry expires, it remains in the cache (both the segment that it belongs to, and the LRU list) until an eviction occurs. The problem here is that the compute if absent implementation relies on there not being an association to a key that we are trying to put because it internally uses put if absent on the underlying segment. If we try to put an association for a key that has expired but not been evicted, then compute if absent will return as if there is nothing in the cache for the given key, yet no call to compute if absent will succeed in putting a new association for the key. To remedy this, we modify the internal get method for the cache to let the caller take action if the entry they are retrieving is expired. This allows the compute if absent method to take the action of evicting the entry from the cache, thus allowing the put if absent method used by compute if absent to succeed for one of the callers trying to compute if absent a new association.

When a cache entry expires, it remains in the cache (both the segment
that it belongs to, and the LRU list) until an eviction occurs. The
problem here is that the compute if absent implementation relies on
there not being an association to a key that we are trying to put
because it internally uses put if absent on the underlying segment. If
we try to put an association for a key that has expired but not been
evicted, then compute if absent will return as if there is nothing in
the cache for the given key, yet no call to compute if absent will
succeed in putting a new association for the key. To remedy this, we
modify the internal get method for the cache to let the caller take
action if the entry they are retrieving is expired. This allows the
compute if absent method to take the action of evicting the entry from
the cache, thus allowing the put if absent method used by compute if
absent to succeed for one of the callers trying to compute if absent a
new association.
@dakrone
dakrone approved these changes Sep 6, 2017
Copy link
Member

dakrone left a comment

LGTM left one super minor comment

@@ -360,7 +366,12 @@ private V get(K key, long now) {
*/
public V computeIfAbsent(K key, CacheLoader<K, V> loader) throws ExecutionException {
long now = now();
V value = get(key, now);
// we have to eagerly evict or our putIfAbsent call below will fail

This comment has been minimized.

Copy link
@dakrone

dakrone Sep 6, 2017

Member

"have to eagerly evict or ..." -> "have to eagerly evict expired values or ..."

@jasontedor jasontedor merged commit 9c795bd into elastic:master Sep 6, 2017
1 of 2 checks passed
1 of 2 checks passed
elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
jasontedor added a commit that referenced this pull request Sep 6, 2017
When a cache entry expires, it remains in the cache (both the segment
that it belongs to, and the LRU list) until an eviction occurs. The
problem here is that the compute if absent implementation relies on
there not being an association to a key that we are trying to put
because it internally uses put if absent on the underlying segment. If
we try to put an association for a key that has expired but not been
evicted, then compute if absent will return as if there is nothing in
the cache for the given key, yet no call to compute if absent will
succeed in putting a new association for the key. To remedy this, we
modify the internal get method for the cache to let the caller take
action if the entry they are retrieving is expired. This allows the
compute if absent method to take the action of evicting the entry from
the cache, thus allowing the put if absent method used by compute if
absent to succeed for one of the callers trying to compute if absent a
new association.

Relates #26516
jasontedor added a commit that referenced this pull request Sep 6, 2017
When a cache entry expires, it remains in the cache (both the segment
that it belongs to, and the LRU list) until an eviction occurs. The
problem here is that the compute if absent implementation relies on
there not being an association to a key that we are trying to put
because it internally uses put if absent on the underlying segment. If
we try to put an association for a key that has expired but not been
evicted, then compute if absent will return as if there is nothing in
the cache for the given key, yet no call to compute if absent will
succeed in putting a new association for the key. To remedy this, we
modify the internal get method for the cache to let the caller take
action if the entry they are retrieving is expired. This allows the
compute if absent method to take the action of evicting the entry from
the cache, thus allowing the put if absent method used by compute if
absent to succeed for one of the callers trying to compute if absent a
new association.

Relates #26516
jasontedor added a commit that referenced this pull request Sep 6, 2017
When a cache entry expires, it remains in the cache (both the segment
that it belongs to, and the LRU list) until an eviction occurs. The
problem here is that the compute if absent implementation relies on
there not being an association to a key that we are trying to put
because it internally uses put if absent on the underlying segment. If
we try to put an association for a key that has expired but not been
evicted, then compute if absent will return as if there is nothing in
the cache for the given key, yet no call to compute if absent will
succeed in putting a new association for the key. To remedy this, we
modify the internal get method for the cache to let the caller take
action if the entry they are retrieving is expired. This allows the
compute if absent method to take the action of evicting the entry from
the cache, thus allowing the put if absent method used by compute if
absent to succeed for one of the callers trying to compute if absent a
new association.

Relates #26516
jasontedor added a commit that referenced this pull request Sep 6, 2017
When a cache entry expires, it remains in the cache (both the segment
that it belongs to, and the LRU list) until an eviction occurs. The
problem here is that the compute if absent implementation relies on
there not being an association to a key that we are trying to put
because it internally uses put if absent on the underlying segment. If
we try to put an association for a key that has expired but not been
evicted, then compute if absent will return as if there is nothing in
the cache for the given key, yet no call to compute if absent will
succeed in putting a new association for the key. To remedy this, we
modify the internal get method for the cache to let the caller take
action if the entry they are retrieving is expired. This allows the
compute if absent method to take the action of evicting the entry from
the cache, thus allowing the put if absent method used by compute if
absent to succeed for one of the callers trying to compute if absent a
new association.

Relates #26516
@jasontedor jasontedor deleted the jasontedor:cache-compute-if-absent branch Sep 6, 2017
@jasontedor

This comment has been minimized.

Copy link
Member Author

jasontedor commented Sep 6, 2017

Thanks @dakrone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.