Dan Burkert has posted comments on this change. Change subject: util: add file cache ......................................................................
Patch Set 8: (4 comments) only 1/2 way through but going to switch to r8 for the rest. http://gerrit.cloudera.org:8080/#/c/5146/6/src/kudu/util/file_cache-stress-test.cc File src/kudu/util/file_cache-stress-test.cc: Line 133: deque<shared_ptr<FileType>> files; It took me a bit to figure out the difference between this and the global pool of files -- you may want to add a comment like "Active opened files in this consumer thread" http://gerrit.cloudera.org:8080/#/c/5146/6/src/kudu/util/file_cache-test-util.h File src/kudu/util/file_cache-test-util.h: Line 68: CHECK_LE(open_fd_count, max_fd_count); This should include an error message, since the LOG_EVERY_N may not have fired since the count was less than the max. http://gerrit.cloudera.org:8080/#/c/5146/6/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: Line 72: void AssertFdsAndDescriptors(int num_expected_fds, at every call site you are offsetting by this->initial_open_fds_, maybe the method could do that automatically? Line 101: { Is this scope necessary? -- To view, visit http://gerrit.cloudera.org:8080/5146 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I26d02f71b0a9644de0b669875941adae5f426345 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
