Alexey Serbin has posted comments on this change. Change subject: util: add file cache ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/5146/4/src/kudu/util/file_cache.cc File src/kudu/util/file_cache.cc: PS4, Line 57: DCHECK_EQ(':', cache_key[3]) nit: for safety, consider adding a check for 'cache_key' size, so semantically it would be something like DCHECK(cache_key.size() > 3 && cache_key[3] == ':'); PS4, Line 127: Cache::EvictionCallback* eviction_cb() const { nit: does it make sense to make this method private? PS4, Line 206: virtual nit for here and elsewhere while overriding a virtual mathod: keeping 'virtual' along with 'override' seems to be too verbose. May be, drop 'virtual'? Or it is required by the code style? PS4, Line 393: FileCache::~FileCache() { : } nit: consider using '~FileCache() = default;' in the header file instead PS4, Line 499: "rwf" nit: does it make sense to use kRWFileCacheKeyPrefix and kRandomAccessFileCacheKeyPrefix constants here instead? PS4, Line 556: IsFileMarkedForDeletionUnlocked nit: why not to return boolean instead? It might be a bit cheaper, since Status is a non-trivial object. -- 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: 4 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
