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 expire after access #24546

Merged
merged 2 commits into from May 8, 2017

Conversation

Projects
None yet
3 participants
@jasontedor
Member

jasontedor commented May 8, 2017

This commit fixes a bug in the cache expire after access implementation. The bug is this: if you construct a cache with an expire after access of T, put a key, and then touch the key at some time t > T, the act of getting the key would update the access time for the entry before checking if the entry was expired. There are situations in which expire after access would be honored (e.g., if the cache needs to prune the LRU list to keep the cache under a certain weight, or a manual refresh was called) but this behavior is otherwise broken.

Fix cache expire after access
This commit fixes a bug in the cache expire after access
implementation. The bug is this: if you construct a cache with an expire
after access of T, put a key, and then touch the key at some time t > T,
the act of getting the key would update the access time for the entry
before checking if the entry was expired. There are situations in which
expire after access would be honored (e.g., if the cache needs to prune
the LRU list to keep the cache under a certain weight, or a manual
refresh was called) but this behavior is otherwise broken.
@jaymode

jaymode approved these changes May 8, 2017

left a suggestion for improving javadocs. OTT LGTM

* @param now the access time of this entry
* @param key the key of the entry to get from the cache
* @param now the access time of this entry
* @param isExpired test if the entry is expired

This comment has been minimized.

@jaymode

jaymode May 8, 2017

Member

Maybe add something about how if the entry is expired, it is not removed from the cache but is instead ignored?

This comment has been minimized.

@jasontedor

jasontedor May 8, 2017

Member

Good. I pushed 5244699.

@jasontedor jasontedor merged commit 0ec30eb into elastic:master May 8, 2017

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 May 8, 2017

Fix cache expire after access
This commit fixes a bug in the cache expire after access
implementation. The bug is this: if you construct a cache with an expire
after access of T, put a key, and then touch the key at some time t > T,
the act of getting the key would update the access time for the entry
before checking if the entry was expired. There are situations in which
expire after access would be honored (e.g., if the cache needs to prune
the LRU list to keep the cache under a certain weight, or a manual
refresh was called) but this behavior is otherwise broken.

Relates #24546

jasontedor added a commit that referenced this pull request May 8, 2017

Fix cache expire after access
This commit fixes a bug in the cache expire after access
implementation. The bug is this: if you construct a cache with an expire
after access of T, put a key, and then touch the key at some time t > T,
the act of getting the key would update the access time for the entry
before checking if the entry was expired. There are situations in which
expire after access would be honored (e.g., if the cache needs to prune
the LRU list to keep the cache under a certain weight, or a manual
refresh was called) but this behavior is otherwise broken.

Relates #24546
@jasontedor

This comment has been minimized.

Member

jasontedor commented May 8, 2017

Thanks @jaymode.

@jasontedor jasontedor deleted the jasontedor:cache-expire-after-access branch May 8, 2017

debadair added a commit to debadair/elasticsearch that referenced this pull request May 16, 2017

Fix cache expire after access
This commit fixes a bug in the cache expire after access
implementation. The bug is this: if you construct a cache with an expire
after access of T, put a key, and then touch the key at some time t > T,
the act of getting the key would update the access time for the entry
before checking if the entry was expired. There are situations in which
expire after access would be honored (e.g., if the cache needs to prune
the LRU list to keep the cache under a certain weight, or a manual
refresh was called) but this behavior is otherwise broken.

Relates elastic#24546

@clintongormley clintongormley added v6.0.0-alpha2 and removed v6.0.0 labels Jun 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment