Adar Dembo 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 semantica Done PS4, Line 127: Cache::EvictionCallback* eviction_cb() const { > nit: does it make sense to make this method private? It's actually only used in one place, so I'll just do the dereferencing there. PS4, Line 206: virtual > nit for here and elsewhere while overriding a virtual mathod: keeping 'virt I'll drop 'virtual'. PS4, Line 393: FileCache::~FileCache() { : } > nit: consider using '~FileCache() = default;' in the header file instead Done PS4, Line 499: "rwf" > nit: does it make sense to use kRWFileCacheKeyPrefix and kRandomAccessFileC Done PS4, Line 556: IsFileMarkedForDeletionUnlocked > nit: why not to return boolean instead? It might be a bit cheaper, since S Because all three callers do the same thing. They all wrap this in RETURN_NOT_OK(), and they all expect the returned Status (in the event that file is marked for deletion) to be the same. -- 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
