[ 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