Uploaded image for project: 'Pig'
  1. Pig
  2. PIG-3815

Hadoop bug causes to pig to fail silently with jar cache

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.13.0
    • Fix Version/s: 0.13.0
    • Component/s: None
    • Labels:
      None

      Description

      Pig uses DistributedCache.addFileToClassPath api that puts jars on distributed cache configuration. This uses : to separate list of files to be put of classpath via distributed cache. If fs.default.name has port number in it, it causes the tokenization logic to fail in hadoop for retrieving list of cache filenames in backend.

      1. PIG-3815.patch
        2 kB
        Aniket Mokashi
      2. PIG-3815-1.patch
        3 kB
        Aniket Mokashi
      3. PIG-3815-2.patch
        1 kB
        Aniket Mokashi
      4. PIG-3815-3.patch
        2 kB
        Aniket Mokashi

        Issue Links

          Activity

          Hide
          cheolsoo Cheolsoo Park added a comment -
          1. Can you delete this? It's unused.
            +import org.codehaus.plexus.util.IOUtil;
            
          2. Do you mind fixing JobControlCompiler.java#L1700 too? Looks like we can use IOUtils.closeQuietly() here too.
                    OutputStream os = fs.create(dst);
                    try {
                        IOUtils.copyBytes(url.openStream(), os, 4096, true);
                    } finally {
                        // IOUtils can not close both the input and the output properly in a finally
                        // as we can get an exception in between opening the stream and calling the method
                        os.close();
                    }
            
          Show
          cheolsoo Cheolsoo Park added a comment - Can you delete this? It's unused. + import org.codehaus.plexus.util.IOUtil; Do you mind fixing JobControlCompiler.java#L1700 too? Looks like we can use IOUtils.closeQuietly() here too. OutputStream os = fs.create(dst); try { IOUtils.copyBytes(url.openStream(), os, 4096, true ); } finally { // IOUtils can not close both the input and the output properly in a finally // as we can get an exception in between opening the stream and calling the method os.close(); }
          Hide
          julienledem Julien Le Dem added a comment -

          same comment as 1. from Cheolsoo
          otherwise, this looks good to me.

          Show
          julienledem Julien Le Dem added a comment - same comment as 1. from Cheolsoo otherwise, this looks good to me.
          Hide
          aniket486 Aniket Mokashi added a comment -

          Thanks for the review, Julien Le Dem and Cheolsoo Park. I have attached revised patch and committed it to trunk!

          Show
          aniket486 Aniket Mokashi added a comment - Thanks for the review, Julien Le Dem and Cheolsoo Park . I have attached revised patch and committed it to trunk!
          Hide
          rohini Rohini Palaniswamy added a comment -

          Actually I see some issue with this patch. Reopening jira.

          1) Changing os.close() to IOUtils.closeQuietly(os); is not good. You can close the input quietly, but not output especially HDFS outputstream. HDFS can create empty files without data which can be accessed through NN fine if os.close() failed. We have been bitten by this a lot of time. In internal projects, we delete the file and retry if os.close() failed. So please let the pig script fail if os.close() failed rather than causing unexpected behavior.

          2) addFileToClassPath is already doing file.toUri().getPath(). I don't see where the hadoop bug is coming from.

          http://svn.apache.org/viewvc/hadoop/common/branches/branch-1.0/src/mapred/org/apache/hadoop/filecache/DistributedCache.java?revision=1206848&view=markup

          public static void addFileToClassPath
                     (Path file, Configuration conf, FileSystem fs)
                  throws IOException {
              String filepath = file.toUri().getPath();
              String classpath = conf.get("mapred.job.classpath.files");
              conf.set("mapred.job.classpath.files", classpath == null
                  ? filepath
                  : classpath + System.getProperty("path.separator") + filepath);
              URI uri = fs.makeQualified(file).toUri();
              addCacheFile(uri, conf);
            }
          
          Show
          rohini Rohini Palaniswamy added a comment - Actually I see some issue with this patch. Reopening jira. 1) Changing os.close() to IOUtils.closeQuietly(os); is not good. You can close the input quietly, but not output especially HDFS outputstream. HDFS can create empty files without data which can be accessed through NN fine if os.close() failed. We have been bitten by this a lot of time. In internal projects, we delete the file and retry if os.close() failed. So please let the pig script fail if os.close() failed rather than causing unexpected behavior. 2) addFileToClassPath is already doing file.toUri().getPath(). I don't see where the hadoop bug is coming from. http://svn.apache.org/viewvc/hadoop/common/branches/branch-1.0/src/mapred/org/apache/hadoop/filecache/DistributedCache.java?revision=1206848&view=markup public static void addFileToClassPath (Path file, Configuration conf, FileSystem fs) throws IOException { String filepath = file.toUri().getPath(); String classpath = conf.get( "mapred.job.classpath.files" ); conf.set( "mapred.job.classpath.files" , classpath == null ? filepath : classpath + System .getProperty( "path.separator" ) + filepath); URI uri = fs.makeQualified(file).toUri(); addCacheFile(uri, conf); }
          Hide
          aniket486 Aniket Mokashi added a comment -

          Thanks for your comments, Rohini Palaniswamy. I was not aware of limitations on the HDFS streams, I have attached a patch (PIG-3815-2.patch) to fix those problems.

          Hadoop Jira: https://issues.apache.org/jira/browse/MAPREDUCE-2361. Looks like this was fixed here - http://svn.apache.org/viewvc?view=revision&revision=1077790.

          Show
          aniket486 Aniket Mokashi added a comment - Thanks for your comments, Rohini Palaniswamy . I was not aware of limitations on the HDFS streams, I have attached a patch ( PIG-3815 -2.patch) to fix those problems. Hadoop Jira: https://issues.apache.org/jira/browse/MAPREDUCE-2361 . Looks like this was fixed here - http://svn.apache.org/viewvc?view=revision&revision=1077790 .
          Hide
          julienledem Julien Le Dem added a comment -

          Rohini Palaniswamy in the code you quoted, don't you think it is putting the port back in the following line?

          URI uri = fs.makeQualified(file).toUri();
          
          Show
          julienledem Julien Le Dem added a comment - Rohini Palaniswamy in the code you quoted, don't you think it is putting the port back in the following line? URI uri = fs.makeQualified(file).toUri();
          Hide
          rohini Rohini Palaniswamy added a comment -

          Yes. It has been fixed 3 years ago. I am not sure what version of hadoop you are using and hitting this issue. But since we still support 0.20 as well there is no harm in doing .toUri().getPath() in pig as well.

          +1. Since the issue is not with hadoop 1.0, please update your comment when checking in this patch from "// PIG-3815 In hadoop 1.0, addFileToClassPath uses : as separator" to say hadoop 0.20.

          Show
          rohini Rohini Palaniswamy added a comment - Yes. It has been fixed 3 years ago. I am not sure what version of hadoop you are using and hitting this issue. But since we still support 0.20 as well there is no harm in doing .toUri().getPath() in pig as well. +1. Since the issue is not with hadoop 1.0, please update your comment when checking in this patch from "// PIG-3815 In hadoop 1.0, addFileToClassPath uses : as separator" to say hadoop 0.20.
          Hide
          rohini Rohini Palaniswamy added a comment -

          Julien Le Dem,
          It is being qualified only to be used in addCacheFile() which sets the mapred.cache.files which is required. conf.set("mapred.job.classpath.files") uses just the file path after removing scheme and port.

          Show
          rohini Rohini Palaniswamy added a comment - Julien Le Dem , It is being qualified only to be used in addCacheFile() which sets the mapred.cache.files which is required. conf.set("mapred.job.classpath.files") uses just the file path after removing scheme and port.
          Hide
          aniket486 Aniket Mokashi added a comment -

          I have committed PIG-3815-2.patch to trunk! Thanks everyone for your comments.

          Show
          aniket486 Aniket Mokashi added a comment - I have committed PIG-3815 -2.patch to trunk! Thanks everyone for your comments.
          Hide
          aniket486 Aniket Mokashi added a comment -

          I just realized that there is a better way to refactor this code. Can someone review the patch attached?

          Show
          aniket486 Aniket Mokashi added a comment - I just realized that there is a better way to refactor this code. Can someone review the patch attached?
          Hide
          cheolsoo Cheolsoo Park added a comment -

          Ha, that looks a lot better to me. +1.

          Show
          cheolsoo Cheolsoo Park added a comment - Ha, that looks a lot better to me. +1.
          Hide
          aniket486 Aniket Mokashi added a comment -

          Thanks Cheolsoo Park, I committed it to trunk.

          Show
          aniket486 Aniket Mokashi added a comment - Thanks Cheolsoo Park , I committed it to trunk.

            People

            • Assignee:
              aniket486 Aniket Mokashi
              Reporter:
              aniket486 Aniket Mokashi
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development