Adar Dembo 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<> I'm gonna punt on this; it's not in-scope for this patch. 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 a Done PS8, Line 45: Start() > Are any guardrails needed to check if the Start() method is not called more Done 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. Done 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? You mean, that the in-cache file handle belonging to f2 was evicted from the cache and closed? Yeah, I can do that. PS8, Line 173: DeleteFile(kFile2) > What happens if the file is moved/renamed when it's opened by the cache? I Moving/renaming a file is totally out of scope, and not an operation done by block managers. If you move a file opened by the cache, I'm sure bad things will happen, as the filename is used as the cache key and is static from the moment the file is opened. PS8, Line 228: 0 > Is the offset set to 0 intentionally? Does it make sense to vary the offse I don't think it matters. The cache certainly doesn't care (the parameters are just passed through). 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? I had some down_casts in here at one point, but no longer. PS8, Line 164: handle_.get(); > Why not just Apparently that doesn't work: /home/adar/Source/kudu/src/kudu/util/file_cache.cc: In instantiation of ‘bool kudu::internal::ScopedOpenedDescriptor<FileType>::opened() const [with FileType = kudu::RWFile]’: /home/adar/Source/kudu/src/kudu/util/file_cache.cc:272:22: required from here /home/adar/Source/kudu/src/kudu/util/file_cache.cc:170:32: error: cannot convert ‘const UniqueHandle {aka const std::unique_ptr<kudu::Cache::Handle, kudu::Cache::HandleDeleter>}’ to ‘bool’ in return bool opened() const { 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
