[ https://issues.apache.org/jira/browse/HDFS-5810?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13897170#comment-13897170 ]
Andrew Wang commented on HDFS-5810: ----------------------------------- Hi Colin, some replies and new comments. I looked at the remaining parts of the previous patch, I haven't looked at the newest rev yet: Replies: bq. <mmap.cache.timeout> Sure, let's just doc it. bq. polymorphic Object in SCReplica Sure, this is just a style nit. If you tried it the other way and it looked worse, it's fine to leave it as is. bq. <mmap and global lock> I guess this is for my own edification, but isn't munmap going to be approximately the same cost as mmap? Both involve updating the page tables and a TLB flush AFAIK, which should be order microseconds. This could be pushed up to milliseconds if the page tables are swapped out, but that's again an issue for both. I'd like to be internally consistent with regard to our locking, if it's a performance argument. Overall, I feel like microseconds are not a big deal, and mmap/munmap themselves have to grab a kernel lock. The code savings from removing the CV also aren't bad, since we could reduce the polymorphism of SCReplica#mmapData. Some new comments too (I think I've looked at all the changed files at this point): ClientContext: * ClientContext#confAsString has a dupe of socketCacheExpiry. Do we also need the mmap cache settings here? * ClientContext#getFromConf, can we push the creation of a new DFSClient.Conf into #get when it's necessary? Seems better to avoid doing all those hash lookups. BlockReaderFactory: * We removed the javadoc parameter descriptions in a few places, some of which were helpful (e.g. {{len}} of {{-1}} means read as many bytes as possible). Could we add the one-line docs back to the builder variables? * Mind adding "dfs.client.cached.conn.retry" to hdfs-default.xml? * cacheTries now counts down instead of counting up, so I think it needs a new name. cacheTriesRemaining isn't great, but something like that. * cacheTries used to also only tick when we got a stale peer out of the cache. Now, nextTcpPeer and nextDomainPeer tick cacheTries unconditionally. * Previously, we would disable domain sockets or throw an exception if we hit an error when using a new Peer (domain or TCP respectively). Now, we don't know if a peer is cached or new, and spin until we run out of cacheTries (which isn't really related here). > 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, > HDFS-5810.016.patch, HDFS-5810.018.patch, HDFS-5810.019.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)