Tim Armstrong has posted comments on this change. Change subject: IMPALA-5352: Age out unused file handles from the cache ......................................................................
Patch Set 1: (8 comments) No serious concerns, just some thoughts on simplification, etc. http://gerrit.cloudera.org:8080/#/c/7640/1/be/src/runtime/disk-io-mgr-handle-cache.h File be/src/runtime/disk-io-mgr-handle-cache.h: Line 170: /// Periodic check to evict unused file handles maybe mention "executed by eviction_thread_". Line 178: size_t total_capacity_; total_capacity_ could do with a short comment. Alternatively I'm not sure that it's really needed, since it's only used in one place. Line 187: bool shut_down_ = false; It would be better to use an atomic so that it's obviously correct and there aren't any subtleties about when the write to the variable is actually visible to the eviction thread. http://gerrit.cloudera.org:8080/#/c/7640/1/be/src/runtime/disk-io-mgr-handle-cache.inline.h File be/src/runtime/disk-io-mgr-handle-cache.inline.h: Line 76: if (unused_handle_timeout_secs_ != 0 && total_capacity_ != 0) { It's worth considering if we should just always run the thread for simplicity, since it's needed in the default configuration and overhead shouldn't be too bad. PS1, Line 187: 1000 Might be helpful to make this a constant in the class. PS1, Line 197: true Can we combine this line and the one below? http://gerrit.cloudera.org:8080/#/c/7640/1/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: PS1, Line 101: insures nit: ensures http://gerrit.cloudera.org:8080/#/c/7640/1/tests/custom_cluster/test_hdfs_fd_caching.py File tests/custom_cluster/test_hdfs_fd_caching.py: PS1, Line 147: 60 Maybe extract into a local variable and mention that it's derived from unused_file_handle_timeout_mins. Otherwise requires a bit of work to figure out where the number came from. Could we make the unused_file_handle_timeout_mins argument a floating point value and decrease the runtime of this test? This is run serially so this test alone will add over a minute to the build time. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
