Adar Dembo has posted comments on this change. Change subject: util: add file cache ......................................................................
Patch Set 8: (5 comments) 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 p Done 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 fi We'll already get one for free that'll include both open_fd_count and max_fd_count. That's the entirety of the KLOG_EVERY(). 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 Done Line 101: { > Is this scope necessary? Done http://gerrit.cloudera.org:8080/#/c/5146/8/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: Line 177: ScopedOpenedDescriptor<FileType> BaseDescriptor<FileType>::InsertIntoCache( > Any reason not to put this method and the one below inline with the class? It's because they return ScopedOpenedDescriptors. That means we need to observe the following order: 1) Forward declare ScopedOpenedDescriptor. 2) Declare BaseDescriptor, which declares these two methods. 3) Declare ScopedOpenedDescriptor. We also define it inline, but that's orthogonal to this problem. 4) Define these BaseDescriptor methods, which we can do now that ScopedOpenedDescriptor has been declared. ...I wrote all that, then I tried inlining it anyway. It worked, provided ScopedOpenedDescriptor was forward declared. I guess I don't know how compilers work. :/ -- 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
