Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: tools/rumen
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      TestRumenJobTraces failed with following error:
      Error Message

      the gold file contains more text at line 1 expected:<56> but was:<0>

      Stacktrace

      at org.apache.hadoop.tools.rumen.TestRumenJobTraces.testHadoop20JHParser(TestRumenJobTraces.java:294)

      Full log of the failure is available at http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/292/testReport/org.apache.hadoop.tools.rumen/TestRumenJobTraces/testHadoop20JHParser/

      1. 1925.v2.patch
        15 kB
        Ravi Gummadi
      2. 1925.v2.1.patch
        15 kB
        Ravi Gummadi
      3. 1925.v1.patch
        11 kB
        Ravi Gummadi
      4. 1925.v1.1.patch
        12 kB
        Ravi Gummadi
      5. 1925.patch
        1 kB
        Ravi Gummadi

        Activity

        Hide
        Amar Kamat added a comment -

        Looked at if briefly and it seems that there is some issue with mark and reset on compressed files. TestRumenJobTraces.testHadoop20JHParser() uses PossiblyDecompressedInputStream for reading the compressed v20-single-input-log.gz file. I commented out the code to do with reset and the test passed. For now one possible culprit is HADOOP-6835.

        Show
        Amar Kamat added a comment - Looked at if briefly and it seems that there is some issue with mark and reset on compressed files. TestRumenJobTraces.testHadoop20JHParser() uses PossiblyDecompressedInputStream for reading the compressed v20-single-input-log.gz file. I commented out the code to do with reset and the test passed. For now one possible culprit is HADOOP-6835 .
        Hide
        Hong Tang added a comment -

        PossiblyDecompressedInputStream.close() seems to be wrong. it should close coreInputStream before returning back the decompressor.

        The following is also problematic:

        
            BufferedInputStream bis = new BufferedInputStream(inputLogStream);
            bis.mark(10000);
            Hadoop20JHParser parser = new Hadoop20JHParser(bis);
        

        The default buffer size is 8K, so mark a position with a read-ahead limit of 10000 characters could fail if the underlying stream does too much read ahead.

        Show
        Hong Tang added a comment - PossiblyDecompressedInputStream.close() seems to be wrong. it should close coreInputStream before returning back the decompressor. The following is also problematic: BufferedInputStream bis = new BufferedInputStream(inputLogStream); bis.mark(10000); Hadoop20JHParser parser = new Hadoop20JHParser(bis); The default buffer size is 8K, so mark a position with a read-ahead limit of 10000 characters could fail if the underlying stream does too much read ahead.
        Hide
        Hong Tang added a comment -

        However, this should not be a problem because otherwise bis.reset() should throw IOException.

        Also, the following line is better to be moved after bis.reset():

            Hadoop20JHParser parser = new Hadoop20JHParser(bis);
        
        Show
        Hong Tang added a comment - However, this should not be a problem because otherwise bis.reset() should throw IOException. Also, the following line is better to be moved after bis.reset(): Hadoop20JHParser parser = new Hadoop20JHParser(bis);
        Hide
        Ravi Gummadi added a comment -

        Problem is with the following line in the testcase:

        Hadoop20JHParser.canParse(inputLogStream));

        It should be reading from BuffuredInputStream 'bis' instead of inputLogStream — sothat bis.reset() would work.

        Hadoop20JHParser.canParse(bis));

        With this change result.txt file contains events. But these events are more than that are there in the expected events file. The expected events file itself seems to be wrong and needs to be updated to have these events.
        Investigating more why this bug is uncovered only now(after HADOOP-6835) only.

        Show
        Ravi Gummadi added a comment - Problem is with the following line in the testcase: Hadoop20JHParser.canParse(inputLogStream)); It should be reading from BuffuredInputStream 'bis' instead of inputLogStream — sothat bis.reset() would work. Hadoop20JHParser.canParse(bis)); With this change result.txt file contains events. But these events are more than that are there in the expected events file. The expected events file itself seems to be wrong and needs to be updated to have these events. Investigating more why this bug is uncovered only now(after HADOOP-6835 ) only.
        Hide
        Ravi Gummadi added a comment -

        Attaching patch fixing the testcase and the dependent gzipped data file.

        Show
        Ravi Gummadi added a comment - Attaching patch fixing the testcase and the dependent gzipped data file.
        Hide
        Ravi Gummadi added a comment -

        'git diff' couldn't get the changes to the gzipped file in my patch attached.
        I will remove the whole file and will have the expected output in an array in the test case itself — as suggested by Amar offline.

        Show
        Ravi Gummadi added a comment - 'git diff' couldn't get the changes to the gzipped file in my patch attached. I will remove the whole file and will have the expected output in an array in the test case itself — as suggested by Amar offline.
        Hide
        Amar Kamat added a comment -

        Few comments

        1. The patch doesnt contain the changes to the v20-single-input-log-event-classes.text.gz file.
        2. You can get rid of the inputLogStream variable to avoid future confusion
        3. You can make the resulting events list and the gold-standard list, in-memory. Instead of writing the test events into a file (result.txt) and then comparing the contents of 2 files (result.txt and v20-single-input-log-event-classes.text.gz) you can keep the contents of both the files in memory and get rid of result.txt and v20-single-input-log-event-classes.text.gz. The test will be faster and also easy to change in future.
        4. If you decide to do the above then there is no need of tempDir and rootTempDir.
        Show
        Amar Kamat added a comment - Few comments The patch doesnt contain the changes to the v20-single-input-log-event-classes.text.gz file. You can get rid of the inputLogStream variable to avoid future confusion You can make the resulting events list and the gold-standard list, in-memory. Instead of writing the test events into a file ( result.txt ) and then comparing the contents of 2 files ( result.txt and v20-single-input-log-event-classes.text.gz ) you can keep the contents of both the files in memory and get rid of result.txt and v20-single-input-log-event-classes.text.gz . The test will be faster and also easy to change in future. If you decide to do the above then there is no need of tempDir and rootTempDir .
        Hide
        Hong Tang added a comment -

        Git diff --text will add binary diff to the patch.

        Show
        Hong Tang added a comment - Git diff --text will add binary diff to the patch.
        Hide
        Ravi Gummadi added a comment -

        Thanks Hong.
        Will upload new patch which removes that .gz file and the testcase itself contains the expected list of events as array of Strings.

        Show
        Ravi Gummadi added a comment - Thanks Hong. Will upload new patch which removes that .gz file and the testcase itself contains the expected list of events as array of Strings.
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch incorporating review comments.

        Show
        Ravi Gummadi added a comment - Attaching new patch incorporating review comments.
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch for current trunk. Updates the testcases refactored and made more readable.

        Show
        Ravi Gummadi added a comment - Attaching new patch for current trunk. Updates the testcases refactored and made more readable.
        Hide
        Hong Tang added a comment -

        The patch looks good - the test case looks much cleaner now.

        Could you also accommodate a few minor issues I found?

        • Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream. Rationale: CompressionInputStream.close() may still access the decompressor.
        • Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);"
        • Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset(). Rationale: in case in the future this constructor may read a few bytes from the stream.
        Show
        Hong Tang added a comment - The patch looks good - the test case looks much cleaner now. Could you also accommodate a few minor issues I found? Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream. Rationale: CompressionInputStream.close() may still access the decompressor. Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);" Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset(). Rationale: in case in the future this constructor may read a few bytes from the stream.
        Hide
        Amar Kamat added a comment -

        Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream.

        CodecPool.returnDecompressor() internally calls decompressor.reset(). Wouldn't this also potentially access the underlying I/O stream?

        Explicitly make the buffer size and mark limit consistent.

        Is this just for the testcase or for the Rumen framework code too?

        Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset().

        Isn't this what Ravi has done?

        Hadoop20JHParser parser = null;
        try {
        ...
        ..
        bis.reset();
        parser = new Hadoop20JHParser(bis);
        ...
        ..
        }
        
        Show
        Amar Kamat added a comment - Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream. CodecPool.returnDecompressor() internally calls decompressor.reset() . Wouldn't this also potentially access the underlying I/O stream? Explicitly make the buffer size and mark limit consistent. Is this just for the testcase or for the Rumen framework code too? Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset(). Isn't this what Ravi has done? Hadoop20JHParser parser = null ; try { ... .. bis.reset(); parser = new Hadoop20JHParser(bis); ... .. }
        Hide
        Ravi Gummadi added a comment -
        • Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream. Rationale: CompressionInputStream.close() may still access the decompressor.

        OK. Will do this.

        • Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);"

        I didn't get why these 2 should be same. As default BUFSIZE is 8K, we just need some number > 8k and the testcase is using 10000. Isn't this OK ? Any issues ? Just to be on the safer side, if you want it to be increased to a bigger value like 32K(from 10000), I can do so. Do you see any gain because of having these 2 same ?

        • Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset(). Rationale: in case in the future this constructor may read a few bytes from the stream.

        This was already incorporated in my latest patch 1925.v1.1.patch.

        Show
        Ravi Gummadi added a comment - Invert the order of closing the input stream and returning the the decompressor in PossiblyDecompressedInputStream. Rationale: CompressionInputStream.close() may still access the decompressor. OK. Will do this. Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);" I didn't get why these 2 should be same. As default BUFSIZE is 8K, we just need some number > 8k and the testcase is using 10000. Isn't this OK ? Any issues ? Just to be on the safer side, if you want it to be increased to a bigger value like 32K(from 10000), I can do so. Do you see any gain because of having these 2 same ? Move this "Hadoop20JHParser parser = new Hadoop20JHParser(bis);" after bis.reset(). Rationale: in case in the future this constructor may read a few bytes from the stream. This was already incorporated in my latest patch 1925.v1.1.patch.
        Hide
        Amar Kamat added a comment -
        • Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);"

        I didn't get why these 2 should be same.

        Can we modify the testcase to make use of RewindableInputStream? Rumen actually uses RewindableInputStream to take care of this mark/reset business.

        Show
        Amar Kamat added a comment - Explicitly make the buffer size and mark limit consistent. E.g. "static final int BUFSIZE = 10000; BufferedInputStream bis = new BufferedInputStream(inputLogStream, BUFSIZE); bis.mark(BUFSIZE);" I didn't get why these 2 should be same. Can we modify the testcase to make use of RewindableInputStream ? Rumen actually uses RewindableInputStream to take care of this mark/reset business.
        Hide
        Hong Tang added a comment -

        CodecPool.returnDecompressor() internally calls decompressor.reset(). Wouldn't this also potentially access the underlying I/O stream?

        No, Decompressor.reset() clears its state so that it can be reused to decompress another stream of input. A bug in a similar nature is HADOOP-4195.

        I didn't get why these 2 should be same. As default BUFSIZE is 8K, we just need some number > 8k and the testcase is using 10000. Isn't this OK ? Any issues ? Just to be on the safer side, if you want it to be increased to a bigger value like 32K(from 10000), I can do so. Do you see any gain because of having these 2 same ?

        That is fine too. I would even prefer to have the code to reflect the nature that the buffer size must be no less than the mark limit through something like: bis.mark(Math.min(BUFSIZE, 10000)).

        Can we modify the testcase to make use of RewindableInputStream? Rumen actually uses RewindableInputStream to take care of this mark/reset business.

        That is also fine with me.

        This was already incorporated in my latest patch 1925.v1.1.patch.

        Thanks!

        Show
        Hong Tang added a comment - CodecPool.returnDecompressor() internally calls decompressor.reset(). Wouldn't this also potentially access the underlying I/O stream? No, Decompressor.reset() clears its state so that it can be reused to decompress another stream of input. A bug in a similar nature is HADOOP-4195 . I didn't get why these 2 should be same. As default BUFSIZE is 8K, we just need some number > 8k and the testcase is using 10000. Isn't this OK ? Any issues ? Just to be on the safer side, if you want it to be increased to a bigger value like 32K(from 10000), I can do so. Do you see any gain because of having these 2 same ? That is fine too. I would even prefer to have the code to reflect the nature that the buffer size must be no less than the mark limit through something like: bis.mark(Math.min(BUFSIZE, 10000)). Can we modify the testcase to make use of RewindableInputStream? Rumen actually uses RewindableInputStream to take care of this mark/reset business. That is also fine with me. This was already incorporated in my latest patch 1925.v1.1.patch. Thanks!
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch incorporating Hong's comments.

        Show
        Ravi Gummadi added a comment - Attaching new patch incorporating Hong's comments.
        Hide
        Hong Tang added a comment -

        One minor nit: Can we avoid involving InputDemuxer in getRewindableInputStream? It probably suffice to create a FSDataInputStream and create a RewindableInputStream on top of it. Other than that, the patch looks good.

        Show
        Hong Tang added a comment - One minor nit: Can we avoid involving InputDemuxer in getRewindableInputStream? It probably suffice to create a FSDataInputStream and create a RewindableInputStream on top of it. Other than that, the patch looks good.
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch removing the dependency of InputDemuxer in getRewindableInputStream().

        Show
        Ravi Gummadi added a comment - Attaching new patch removing the dependency of InputDemuxer in getRewindableInputStream().
        Hide
        Ravi Gummadi added a comment -

        Hudson seems to be not responding.
        I ran ant test and test-patch myself.

        All unit tests passed except the known failure of MAPREDUCE-1834.

        ant test-patch gave:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 5 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

        Show
        Ravi Gummadi added a comment - Hudson seems to be not responding. I ran ant test and test-patch myself. All unit tests passed except the known failure of MAPREDUCE-1834 . ant test-patch gave: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 5 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        Hide
        Hong Tang added a comment -

        Patch looks good to me. +1.

        Show
        Hong Tang added a comment - Patch looks good to me. +1.
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Ravi!

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Ravi!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #523 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/523/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #523 (See https://hudson.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/523/ )

          People

          • Assignee:
            Ravi Gummadi
            Reporter:
            Amareshwari Sriramadasu
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development