hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Nauroth (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MAPREDUCE-5791) Shuffle phase is slow in Windows - FadviseFileRegion::transferTo does not read disks efficiently
Date Mon, 17 Mar 2014 18:13:45 GMT

    [ https://issues.apache.org/jira/browse/MAPREDUCE-5791?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13938139#comment-13938139

Chris Nauroth commented on MAPREDUCE-5791:

Thanks for the update, [~nikola.vujic].  A few more thoughts on this patch:

# {{mapred-default.xml}}
## I think by setting {{mapreduce.shuffle.transferTo.allowed}} to {{true}} in mapred-default.xml,
you'll end up getting transferTo turned on in Windows by default.  The value defined in the
XML takes precedence over the default in the code.  Instead, if you set the value to nothing/empty
string in the XML, then your logic in {{ShuffleHandler}} will select the right value at runtime
based on OS.  For an existing example of this, see {{mapreduce.application.classpath}}.
# {{FadvisedFileRegion}}
## It appears that {{customShuffleTransfer}} is public only to make it accessible for tests.
 Is that correct?  If so, would you please change this to package-private (drop public) and
add the {{com.google.common.annotations.VisibleForTesting}} annotation on the method?
## At the start of {{customShuffleTransfer}}, there is some indentation by 4 spaces in front
of the {{throw}} and the {{return 0L;}}.  Would you please change those to 2 spaces?
## I was a bit confused by this logic:
      //adjust counters and buffer limit
      if(readSize < trans) {
        trans -= readSize;
        position += readSize;
      } else {
        position += trans; 
        trans = 0;
I don't understand why we need the conditional.  Looking specifically at the else block, setting
limit to current position and setting current position to 0 is the same effect as calling
{{Buffer#flip}}.  Updating {{position}} looks unnecessary, because setting {{trans}} to 0
forces this to be the final iteration of the loop.  Overall, it appears that this code could
be simplified by dropping the if/else and always running the logic currently in the if block.
 What do you think?  Is there some edge case that I'm missing?
# {{TestFadvisedFileRegion}}
## There is still a risk of a resource leak in the test, because the {{RandomAccessFile}}
constructor can throw an exception.  You could successfully open {{inputFile}}, and then {{targetFile}}
fails with an exception, but then {{inputFile}} would never be closed.  I recommend declaring
{{inputFile}}, {{targetFile}} and {{target}} outside the try block, initialized to null, and
then initialize them first thing inside the try block.

> Shuffle phase is slow in Windows - FadviseFileRegion::transferTo does not read disks
> ------------------------------------------------------------------------------------------------
>                 Key: MAPREDUCE-5791
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-5791
>             Project: Hadoop Map/Reduce
>          Issue Type: Bug
>            Reporter: Nikola Vujic
>            Assignee: Nikola Vujic
>         Attachments: MAPREDUCE-5791.patch, MAPREDUCE-5791.patch
> transferTo method in org.apache.hadoop.mapred.FadvisedFileRegion is using transferTo
method from a FileChannel to transfer data from a disk to socket. This is performing slow
in Windows, slower than in Linux. The reason is that transferTo method for the java.nio is
issuing 32K IO requests all the time. In Windows, these 32K transfers are not optimal and
we don't get the best performance form the underlying IO subsystem. In order to achieve better
performance when reading from the drives, we need to read data in bigger chunks, 512K for

This message was sent by Atlassian JIRA

View raw message