Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache ......................................................................
Patch Set 2: (3 comments) 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 :) 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: 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)); Is this really necessary? I'm not sure how performance sensitive this code is, but I'd think we could get away with using a copy constructor and this fanciness. I'm fine with this too if others feel confident that it's correct and worth it. 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 ? -- 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 <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
