[
https://issues.apache.org/jira/browse/HDFS-4817?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13699650#comment-13699650
]
Aaron T. Myers commented on HDFS-4817:
--------------------------------------
Patch looks pretty good to me, Colin. A few small comments.
# The patch no longer applies cleanly to trunk and will need to be rebased. The
conflicts appear pretty minor.
# I think the changes in BlockLocation are unnecessary, and potentially
incompatible as well.
# I'm not in love with the interface name "CanDropCacheBehindWrites", though I
don't have much of a better suggestion. "CacheDroppableBehindWrites" ?
"WriteCacheDroppable" ? Feel free to ignore this comment.
# Why the asymmetry in naming between FSDataInputStream#setDropBehind and
FSDataOutputStream#setDropCacheBehindWrites? i.e. why not
FSDataInputStream#setDropCacheBehindReads? Or just setDropBehind in both cases?
I personally prefer setDropBehind in both cases - each of those streams are
only for reading or writing, respectively.
# In FSDataInputStream#setDropBehind and
FSDataOutputStream#setDropCacheBehindWrites, recommend you add a LOG.debug(...)
(maybe even a LOG.warn?) to log the ClassCastException if it happens.
# Recommend moving the default no-op versions of setReadahead and setDropBehind
into FSInputStream, so that you don't have to add no-op versions to
FSInputChecker, RawLocalFileSystem, FTPInputStream, S3InputStream,
NativeS3FileSystem, ByteRangeInputStream, and TestStreamFile.
# What's the reason for having CachingStrategy#dropBehind and
CachingStrategy#readahead public? They both have a getter and a setter.
# Is the reason that CachingStrategy doesn't separate dropBehind for reads from
dropBehind for writes because a CachingStrategy will only ever be used by
either an input or an output stream? If so, might want to add a comment to
CachingStrategy to make that more obvious. In general CachingStrategy could
stand to have a class comment.
# Is there any good reason why we only have a single client-side config for
drop behind, i.e. why not have separate read and write drop behind configs?
# Minor point throughout the patch: the asymmetry in camel casing of
"readahead" vs. "dropBehind" kind of bothers me, but that's a bit of a matter
of taste. I don't think "readahead" is one word, but if it is, so shouldn't
"dropbehind"?
Tests look really good, btw.
+1 once these are addressed.
> make HDFS advisory caching configurable on a per-file basis
> -----------------------------------------------------------
>
> Key: HDFS-4817
> URL: https://issues.apache.org/jira/browse/HDFS-4817
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: hdfs-client
> Affects Versions: 3.0.0
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Priority: Minor
> Attachments: HDFS-4817.001.patch, HDFS-4817.002.patch,
> HDFS-4817.004.patch
>
>
> HADOOP-7753 and related JIRAs introduced some performance optimizations for
> the DataNode. One of them was readahead. When readahead is enabled, the
> DataNode starts reading the next bytes it thinks it will need in the block
> file, before the client requests them. This helps hide the latency of
> rotational media and send larger reads down to the device. Another
> optimization was "drop-behind." Using this optimization, we could remove
> files from the Linux page cache after they were no longer needed.
> Using {{dfs.datanode.drop.cache.behind.writes}} and
> {{dfs.datanode.drop.cache.behind.reads}} can improve performance
> substantially on many MapReduce jobs. In our internal benchmarks, we have
> seen speedups of 40% on certain workloads. The reason is because if we know
> the block data will not be read again any time soon, keeping it out of memory
> allows more memory to be used by the other processes on the system. See
> HADOOP-7714 for more benchmarks.
> We would like to turn on these configurations on a per-file or per-client
> basis, rather than on the DataNode as a whole. This will allow more users to
> actually make use of them. It would also be good to add unit tests for the
> drop-cache code path, to ensure that it is functioning as we expect.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
