[
https://issues.apache.org/jira/browse/HDFS-5810?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893685#comment-13893685
]
Colin Patrick McCabe commented on HDFS-5810:
--------------------------------------------
bq. I don't ask this lightly, but I really would like some patch splitting
here. I see a lot of nice refactors, but it's hard to separate out the intent
of the changes. One potential easy split is making BlockReader into a builder,
since it's pretty mechanical and touches a lot of files.
I think splitting that off would be a lot harder than you think. Previously,
the {{BlockReader}} creation logic was all in {{DFSInputStream}}. Pulling it
out while using the old cache APIs would be almost a full rewrite of big parts
of this.
Similarly, splitting off the ClientContext stuff would be difficult, since the
old FD cache was not meant to be shared between threads. I guess we could
split it off and just put PeerCache in there, but I don't think that would
reduce the size of this patch that much.
bq. ClientContext#confAsString needs to be updated to match the ClientContext
members, one dupe too.
updated, thanks
bq. hdfs-default.xml, the default value of stale.threshold.ms of 3000000 does
not match the DFSConfigKeys default value of 1800000.
fixed, thanks
bq. SCReplica#Key is almost identical to FsDatasetCache#Key, but I guess that's
okay unless you want to make a shared BlockKey class.
It's a pretty small class, and we might want to add something to the key in the
future, so I think we should probably keep it as-is.
bq. How did you decide on the default mmap cache size of 1024? I feel like
since we default to 4096 max datanode transfer threads, that'd be a more
appropriate number. If the default FD ulimit of 1024 is your concern, seems
like we should lower it below that for some headroom.
That's a very good question. I guess the answer is that when we did HDFS-4953,
{{dfs.client.mmap.cache.size}} got set to 1024 as a nice round number. I think
you are right that it will cause some problems with running out of file
descriptors with the default max file descriptor ulimit of 1024. It is a shame
to lower the default, but I think that's what we have to do to avoid a poor
user experience.
bq. These caches right now expire purely on a time basis, not actually based on
pressure like, say, the OS page cache. For the mmap cache at least, how about a
hybrid: have a really long timeout, but make eviction also pressure based? The
danger of caching forever seems to be holding on to a "bad fd" (e.g. DN deletes
the file), but with the forthcoming shm communication segment this is mitigated.
I guess the argument in favor of a long value for
{{dfs.client.read.shortcircuit.streams.cache.expiry.ms}} and
{{dfs.client.mmap.cache.timeout.ms}} is that:
* applications should plan for max fd usage anyway, so keeping the cache closer
to full for a while should not be a problem;
* now that the cache is shared between input streams, a longer timeout will
help more threads;
* now that we don't open multiple fds for the same replica, there will be less
FD pressure anyway;
* the kernel memory consumed by open fds is minimal; the page cache evicts
mostly based on usage, not on whether a file is open
argument against is:
* an application may stop using short-circuit reads for a while (or indeed HDFS
entirely), and want those fds back. We have no way for the application to
request them back right now.
Staleness I see as a separate issue. We have two strategies for solving
staleness: timeout-based (for when talking to pre-HDFS-5182 DataNodes), and
shm-based (for talking to newer DataNodes that have HDFS-5182.)
On balance, I agree with your argument. Let's lengthen these timeouts, and
bump up {{dfs.client.read.shortcircuit.streams.cache.size}} a little bit. On
the other hand, the default for {{dfs.client.mmap.cache.size}} needs to be
reduced a bit to avoid overflowing the default fd ulimit.
bq. s/mmaped/mmapped/g, good to be consistent. munmapped too, and case
insensitive.
Fixed
bq. Can use java's built-in TimeUnit to convert rather than the new
nanosecondsToMilliseconds function
OK
bq. In CacheCleaner, rather than a while loop, why not a for each? It'll use
the iterator order, which should be ascending insertion time.
Iterators could get tricky here, since we're calling functions that modify the
eviction maps. I'm not sure what the semantics are for if you call a function
that deletes an element that you're currently looking at an iterator to. It
seems safer just to call firstEntry. The current construction allows us to
refactor this function to release the lock between evictions later, if need be.
bq. Could probably split out the two eviction loops into a single parameterized
function.
I feel like there would be too many parameters. The mmap loop does slightly
different stuff...
bq. ref, unref, fetchOrCreate javadoc say you need to hold the lock, but they
themselves take the cache lock.
Oops-- that's outdated javadoc. Fixed.
bq. close, the try/finally lock idiom takes the lock inside the try, normally
this is outside?
fixed
bq. Propagating a string tag for trace logging is ugly. Can we walk the stack
instead to get the caller, e.g. via ExceptionUtils#getFullStackTrace or other?
I'm okay paying the stack construction cost if it's only for trace logging.
This was something I needed during debugging. For example, for {{unref}}, I
needed to know where the ultimate source was -- {{BlockReaderLocal#close}} or
{{ClientMmap#unref}}. You can't get that just by looking at the top of the
stack-- that will just tell you that {{ShortCircuitCache#purge}} or
{{ShortCircuitReplica#unref}}. Maybe there's a better way to do this... but I
can't think of it right now. I guess we could barf out the whole stack, but
that seems to make tracing a lot harder to read.
bq. allocMmap, no longer a toClose param mentioned in the javadoc
fixed
bq. unref, we could get rid of newRefCount, it doesn't really improve
readability
OK
bq. ShortCircuitReplica#unref calls ShortCircuitCache#unref, but there's no
purge. Will this precondition abort?
The refCount can never be at 0 unless the replica has been purged. Purging
means that the cache no longer contains the replica (and hence does not hold a
reference to it).
On the other hand, purged replicas may have a refCount > 0 if someone is still
using them.
bq. Some references to eviction "lists" but they look to be maps now
Hmm. Quite a few, actually. Fixed.
> Unify mmap cache and short-circuit file descriptor cache
> --------------------------------------------------------
>
> Key: HDFS-5810
> URL: https://issues.apache.org/jira/browse/HDFS-5810
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Affects Versions: 2.3.0
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: HDFS-5810.001.patch, HDFS-5810.004.patch,
> HDFS-5810.006.patch, HDFS-5810.008.patch, HDFS-5810.015.patch
>
>
> We should unify the client mmap cache and the client file descriptor cache.
> Since mmaps are granted corresponding to file descriptors in the cache
> (currently FileInputStreamCache), they have to be tracked together to do
> "smarter" things like HDFS-5182.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
