Adar Dembo has posted comments on this change.

Change subject: util: add file cache
......................................................................


Patch Set 5:

(12 comments)

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 ?
Done


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  ?
This is moot; the code in question has been deleted.


PS5, Line 62: *reinterpret_cast<void**
> I am curious if we need a double indirection first and then access the data
That causes a crash. The reasoning is: the slice's data is a memory address, so 
we need to treat it as a pointer to an RWFile pointer, then dereference the 
first level of indirection to get to the actual RWFile pointer.

If that doesn't make sense, you're in good company; I find this very 
counter-intuitive myself.


PS5, Line 67: *reinterpret_cast<void**
> Same as above.
See above.


PS5, Line 71: EvictionCallback
> Is there a reason to keep this under anonymous namespace and not make it pa
If it's nested in the FileCache, it has to be declared in file_cache.h. I'd 
rather hide that implementation detail, hence the anonymous namespace.


PS5, Line 142: FileCache::
> Basic question: I am curious what's the motive for using the classname as p
This means that ScopedOpenedDescriptor is a nested class within FileCache. I 
used it primarily to deal with member visibility issues, though it's a moot 
point now.


PS5, Line 173: FileCache::ScopedOpenedDescriptor
> Could we use 'using FileCache::ClassMember' at the beginning and just keep 
Moot; it's no longer a nested class.


PS5, Line 275: if (out) {
> What's the use case under which this could be false apart from the very fir
Just that one, but it's legitimate. Suppose you open a descriptor, close it, 
then open it again. The call to ReopenFileIfNecessary() the second time around 
will have a cache hit.


PS5, Line 414: OpenExistingRandomAccessFile
> Nit: I am not a c++ expert, just wondering if we can leverage function over
The Google style guide frowns on using function overloading. In any case, it's 
a moot point now that FileCache is templatized.


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 ?
Possibly. If we've marked the file as deleted, it's because there's an 
outstanding descriptor for it. Forcefully evicting the file from the cache 
could cause it to get reinserted right back by the client upon the next access.

So I don't think deferred deletion should imply anything about the eviction of 
a file.


PS5, Line 80: ihe
> typo
Done


http://gerrit.cloudera.org:8080/#/c/5146/5/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:

PS5, Line 261: readlink(p.c_str(), buf.get(), size)
> Wouldn't it be cheaper to use stat() call instead?  Using stat()/lstat() wo
In retrospect, we're assured that the fd count will be inflated by 1 for 
kProcSelfFd, so we can just subtract it at the end.

Besides, the only safe way to use this function is to call it twice and look at 
the delta.


-- 
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

Reply via email to