Joe McDonnell has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache ......................................................................
Patch Set 2: (12 comments) Also rebased to the latest http://gerrit.cloudera.org:8080/#/c/7640/2//COMMIT_MSG Commit Message: Line 25: This adds a test to custom_cluster/test_hdfs_fd_caching.py > worth thinking about testing this on a cluster where there is a huge burst Since this is happening every second, the cache should have a limited number of file handles to clean up on a single partition in a single iteration. http://gerrit.cloudera.org:8080/#/c/7640/2/be/src/runtime/disk-io-mgr-handle-cache.h File be/src/runtime/disk-io-mgr-handle-cache.h: PS2, Line 97: Returns an error if thread creation fails. > now it's void :) Done http://gerrit.cloudera.org:8080/#/c/7640/2/be/src/runtime/disk-io-mgr-handle-cache.inline.h File be/src/runtime/disk-io-mgr-handle-cache.inline.h: Line 49: shut_down_.Store(SHUTDOWN_FALSE); > nit: I think we can just initialize this in the initialiser list. Done Line 67: if (eviction_thread_.get() != nullptr) { > nit: conditional fits on one line. Done 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? Since this is part of the DiskIoMgr object, disk-io-mgr-test.cc covers this. Some of the tests construct a DiskIoMgr with a smart pointer and later reset that pointer. PS2, Line 116: // This emplace_hint requires constructing a pair in the map. Emplacing an : // element with a multiple argument constructor (such as FileHandleEntry) : // introduces ambiguity between the key's arguments and the value's arguments. : // To resolve the ambiguity, use a piecewise construction and separate the : // arguments for the key from the value. : typename MapType::iterator new_it = p.cache.emplace_hint(range.second, : std::piecewise_construct, std::forward_as_tuple(*fname), : std::forward_as_tuple(new_fh, p.lru_list)); > Yeah I'm wondering if there's a simpler way to express it. Changed this to use a move constructor. This is not performance critical. We can revisit this later if it bothers us. PS2, Line 196: FileHandleCache<NUM_PARTITIONS>::FileHandleCachePartition& p > how about moving this function into FileHandleCachePartition, then calling I added detailed comments about the lru list manipulations, but I don't think putting it in a separate method makes sense. I looked at the function signatures and they would need a lot of explanation in the function comments, so I would rather have the comments inline. My main thought is that this code is one coherent whole, and anyone doing maintenance or improvement on it is going to have to understand every line. I'm reluctant to add code structures to hide anything. Right now, FileHandleCachePartition is purely for memory layout. My general feeling is that adding methods to it and turning FileHandleCachePartition into a full object is overkill. It moves code around, but it doesn't really change anything. PS2, Line 197: uint64_t now = MonotonicSeconds(); > maybe this should be determined in EvictHandlesLoop() before you start taki This is called every second. I don't think there needs to be any consistency between partitions. PS2, Line 198: valid > valid is a bit vague Changed to "allowed" PS2, Line 199: 0 > hitting this branch seems extremely unlikely, only if the user sets the tim Changed the non-eviction test to set the flag to a very large value. 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 Done http://gerrit.cloudera.org:8080/#/c/7640/2/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 112: DEFINE_uint64(unused_file_handle_timeout, 43200, "Maximum time, in seconds, that an " > I do like having the units in the name, would you mind appending _sec ? Done -- To view, visit http://gerrit.cloudera.org:8080/7640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefe04b3e2e22123ecb8b3e494934c93dfb29682e Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
