Dinesh Bhat has posted comments on this change. Change subject: util: add file cache ......................................................................
Patch Set 5: (11 comments) Excellent stuff Adar, took a first pass, still to see tests and deletion path. There are some minor comments and questions below.. http://gerrit.cloudera.org:8080/#/c/5146/5/src/kudu/util/cache.cc File src/kudu/util/cache.cc: PS5, Line 34: Override all cache implementations to use just one shard Can we put a comment describing the motive for this flag ? http://gerrit.cloudera.org:8080/#/c/5146/5/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: PS5, Line 57: > 3 nit: could we compare 'size == 4' here ? PS5, Line 62: *reinterpret_cast<void** I am curious if we need a double indirection first and then access the data pointer using '*' in this casting. i.e, can't we do the casting like "reinterpret_cast<void*>(s.mutable_data())" ? PS5, Line 67: *reinterpret_cast<void** Same as above. PS5, Line 71: EvictionCallback Is there a reason to keep this under anonymous namespace and not make it part of FileCache ? Just to be consistent with other interfaces like CodeCache. PS5, Line 142: FileCache:: Basic question: I am curious what's the motive for using the classname as prefix like that ? Does that mean ScopedOpenedDescriptor can not be member of any other class than FileCache ? PS5, Line 173: FileCache::ScopedOpenedDescriptor Could we use 'using FileCache::ClassMember' at the beginning and just keep ScopedOpedDescriptor for return type ? Here and below ? PS5, Line 275: if (out) { What's the use case under which this could be false apart from the very first cache entry population which is covered by L293 below ? PS5, Line 414: OpenExistingRandomAccessFile Nit: I am not a c++ expert, just wondering if we can leverage function overloading here, since it looks like this routine and above OpenExistingRWFile bear almost same functionality. On the other hand overloading may just give us better readability, so you can as well ignore this comment. http://gerrit.cloudera.org:8080/#/c/5146/5/src/kudu/util/file_cache.h File src/kudu/util/file_cache.h: PS5, Line 75: . Does this need to be in cache any longer when marked to-be-deleted ? PS5, Line 80: ihe typo -- 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: 5 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
