impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Date Fri, 18 Aug 2017 17:24:17 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5352: Age out unused file handles from the cache

Patch Set 2:


some more thoughts
Commit Message:

Line 25: This adds a test to custom_cluster/
worth thinking about testing this on a cluster where there is a huge burst of FHs that need
to get aged out at the same time. the extra time spent cleaning up the structures shouldn't
be significant, but worth checking it doesn't disrupt anything else.
File be/src/runtime/disk-io-mgr-handle-cache.inline.h:

PS2, Line 64: template <size_t NUM_PARTITIONS>
            : FileHandleCache<NUM_PARTITIONS>::~FileHandleCache() {
            :   shut_down_.Store(SHUTDOWN_TRUE);
            :   if (eviction_thread_.get() != nullptr) {
            :     eviction_thread_->Join();
            :   }
            : }
this never actually runs because this has process-lifetime, right?

If we're going to bother with this code, it'd be good to at least have a BE test which exercises
it. (Maybe there already is?)

PS2, Line 96:         // remove from lru and reset iterator
            :         p.lru_list.erase(elem->lru_entry);
            :         elem->lru_entry = p.lru_list.end();
it'd be nice to pull this out into a fn on FileHandleCachePartition and also for the 'returning
fh' case, then the lru manipulation stuff can be kept in one place and hidden from the FileHandleCache.

PS2, Line 172:     DCHECK(release_elem->lru_entry == p.lru_list.end());
             :     release_elem->lru_entry = p.lru_list.emplace(p.lru_list.end(), release_it);
This could be hidden in FileHandleCachePartition as well

PS2, Line 196: FileHandleCache<NUM_PARTITIONS>::FileHandleCachePartition& p
how about moving this function into FileHandleCachePartition, then calling p.EvictHandles(oldest_valid_ts,

It'd be nice to be encapsulating more of the stuff that's touching the lru into FileHandleCachePartition.

PS2, Line 197: uint64_t now = MonotonicSeconds();
maybe this should be determined in EvictHandlesLoop() before you start taking the locks, then
pass it to each call so that each 'run' of the loop applies the same eviction time consistently.

PS2, Line 198: valid
valid is a bit vague

PS2, Line 199: 0
hitting this branch seems extremely unlikely, only if the user sets the timeout to something
absurd. Since this is a code path though, we should have a test case for it. e.g. setting
the flag to max uint64 or something.

PS2, Line 202: lru_entry = p.lru_list.front();
             :     typename MapType::iterator evict_it = lru_entry.map_entry;
             :     uint64_t lru_oldest_timestamp
it'd be nice to use the same naming convention for these vars since they're all associated
with the same oldest element

e.g. oldest_entry, oldest_entry_map_it, oldest_entry_lru_timestamp

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message