impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joe McDonnell (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5352: Age out unused file handles from the cache
Date Sat, 19 Aug 2017 17:57:13 GMT
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 <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message