BlakeOrth commented on code in PR #19140:
URL: https://github.com/apache/datafusion/pull/19140#discussion_r2599597244
##########
datafusion/execution/src/cache/list_files_cache.rs:
##########
@@ -142,46 +173,44 @@ impl DefaultListFilesCacheState {
}
/// Returns the respective entry from the cache, if it exists and the
entry has not expired.
- /// If the entry exists it becomes the most recently used. If the entry
has expired it is
- /// removed from the cache
- fn get(&mut self, key: &Path) -> Option<Arc<Vec<ObjectMeta>>> {
+ /// Takes `now` explicitly to determine expiration.
+ fn get(&mut self, key: &Path, now: Instant) ->
Option<Arc<Vec<ObjectMeta>>> {
let entry = self.lru_queue.get(key)?;
match entry.expires {
- Some(exp) if Instant::now() > exp => {
+ Some(exp) if now > exp => {
self.remove(key);
None
}
_ => Some(Arc::clone(&entry.metas)),
}
}
- /// Checks if the respective entry is currently cached. If the entry has
expired it is removed
- /// from the cache.
- /// The LRU queue is not updated.
- fn contains_key(&mut self, k: &Path) -> bool {
+ /// Checks if the respective entry is currently cached.
+ /// Takes `now` explicitly to determine expiration.
+ fn contains_key(&mut self, k: &Path, now: Instant) -> bool {
let Some(entry) = self.lru_queue.peek(k) else {
return false;
};
match entry.expires {
- Some(exp) if Instant::now() > exp => {
+ Some(exp) if now > exp => {
self.remove(k);
false
}
_ => true,
}
}
- /// Adds a new key-value pair to cache, meaning LRU entries might be
evicted if required.
- /// If the key is already in the cache, the previous entry is returned.
- /// If the size of the entry is greater than the `memory_limit`, the value
is not inserted.
+ /// Adds a new key-value pair to cache.
+ /// Takes `now` explicitly to determine expiration.
Review Comment:
It looks like a decent amount of the doc comment updates here have lost some
communication on the expected output of the method they're documenting. Unless
I'm missing something, taking `now` as a parameter hasn't functionally changed
the behavior of this method. I think including the purpose of `now` is a good
addition, but I'd expect these doc comments to better communicate the purpose
of `now`.
Perhaps they could read like the following:
```rust
/// Adds a new key-value pair to cache, meaning LRU entries might be
evicted if required.
/// If the key is already in the cache, the previous entry is returned.
/// If the size of the entry is greater than the `memory_limit`, the
value is not inserted.
/// The `now` parameter is used to determine the entry's expiration.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
