Alexey Serbin has posted comments on this change. Change subject: util: add file cache ......................................................................
Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/5146/8/src/kudu/util/cache.cc File src/kudu/util/cache.cc: PS8, Line 499: Cache* nit: consider returning std::unique_ptr<> http://gerrit.cloudera.org:8080/#/c/5146/8/src/kudu/util/file_cache-test-util.h File src/kudu/util/file_cache-test-util.h: PS8, Line 40: upper_bound_(upper_bound), nit: may be, replace upper_bound_ with max_fd_count_ and compute it right away given initial_fd_count_ and the upper_bound parameter. PS8, Line 45: Start() Are any guardrails needed to check if the Start() method is not called more than once? http://gerrit.cloudera.org:8080/#/c/5146/8/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: PS8, Line 52: virtual nit: consider dropping 'virtual' since 'override' is present. PS8, Line 131: NO_FATALS(this->AssertFdsAndDescriptors(this->initial_open_fds_ + 1, 2)); Does it make sense to check that f2 is invalidated at this point? PS8, Line 173: DeleteFile(kFile2) What happens if the file is moved/renamed when it's opened by the cache? I hope cache keeps the file open and corresponding operations succeed. Is it worth adding a test for that? PS8, Line 228: 0 Is the offset set to 0 intentionally? Does it make sense to vary the offset for test purposes as well? http://gerrit.cloudera.org:8080/#/c/5146/8/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: PS8, Line 24: #include "kudu/gutil/casts.h" Is this header really needed here? PS8, Line 164: handle_.get(); Why not just return handle_; ? -- 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
