Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-14722

Azure: BlockBlobInputStream position incorrect after seek

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-beta1
    • Component/s: fs/azure
    • Labels:
      None

      Description

      The seek, skip, and getPos methods of BlockBlobInputStream do not correctly account for the stream's internal buffer. This results in invalid stream positions.

      1. HADOOP-14722-006.patch
        11 kB
        Thomas Marquardt
      2. HADOOP-14722-005.patch
        9 kB
        Steve Loughran
      3. HADOOP-14722-003.patch
        9 kB
        Thomas Marquardt
      4. HADOOP-14722-002.patch
        6 kB
        Thomas Marquardt
      5. HADOOP-14722-001.patch
        6 kB
        Thomas Marquardt

        Issue Links

          Activity

          Hide
          tmarquardt Thomas Marquardt added a comment -

          Attaching HADOOP-14722-001.patch.

          The fix includes a new test case (TestBlockBlobInputStream.test_0201_RandomReadTest) which fails without the fix and passes with the fix.

          I ran hadoop-tools/hadoop-azure tests against my endpoint tmarql3. All tests are passing except TestWasbRemoteCallHelper which is tracked by HADOOP-14715 and unrelated to the patch.

          Tests run: 765, Failures: 1, Errors: 0, Skipped 70

          Show
          tmarquardt Thomas Marquardt added a comment - Attaching HADOOP-14722 -001.patch. The fix includes a new test case ( TestBlockBlobInputStream.test_0201_RandomReadTest ) which fails without the fix and passes with the fix. I ran hadoop-tools/hadoop-azure tests against my endpoint tmarql3. All tests are passing except TestWasbRemoteCallHelper which is tracked by HADOOP-14715 and unrelated to the patch. Tests run: 765, Failures: 1, Errors: 0, Skipped 70
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 15m 15s trunk passed
          +1 compile 0m 21s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 22s trunk passed
          +1 findbugs 0m 31s trunk passed
          +1 javadoc 0m 16s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 19s the patch passed
          +1 compile 0m 18s the patch passed
          +1 javac 0m 18s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 27 unchanged - 0 fixed = 28 total (was 27)
          +1 mvnsite 0m 19s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 0m 35s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          +1 javadoc 0m 11s the patch passed
                Other Tests
          +1 unit 2m 6s hadoop-azure in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          22m 48s



          Reason Tests
          FindBugs module:hadoop-tools/hadoop-azure
            org.apache.hadoop.fs.azure.BlockBlobInputStream.seek(long) ignores result of org.apache.hadoop.fs.azure.BlockBlobInputStream.skip(long) At BlockBlobInputStream.java: At BlockBlobInputStream.java:[line 121]



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14722
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879962/HADOOP-14722-001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux bb4d337e8d4d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 6814324
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 15m 15s trunk passed +1 compile 0m 21s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 22s trunk passed +1 findbugs 0m 31s trunk passed +1 javadoc 0m 16s trunk passed       Patch Compile Tests +1 mvninstall 0m 19s the patch passed +1 compile 0m 18s the patch passed +1 javac 0m 18s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 27 unchanged - 0 fixed = 28 total (was 27) +1 mvnsite 0m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 35s hadoop-tools/hadoop-azure generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) +1 javadoc 0m 11s the patch passed       Other Tests +1 unit 2m 6s hadoop-azure in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 22m 48s Reason Tests FindBugs module:hadoop-tools/hadoop-azure   org.apache.hadoop.fs.azure.BlockBlobInputStream.seek(long) ignores result of org.apache.hadoop.fs.azure.BlockBlobInputStream.skip(long) At BlockBlobInputStream.java: At BlockBlobInputStream.java: [line 121] Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14722 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879962/HADOOP-14722-001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux bb4d337e8d4d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6814324 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/artifact/patchprocess/new-findbugs-hadoop-tools_hadoop-azure.html Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12925/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          tmarquardt Thomas Marquardt added a comment -

          Attaching HADOOP-14722-002.patch. This addresses the findbugs warning.

          Show
          tmarquardt Thomas Marquardt added a comment - Attaching HADOOP-14722 -002.patch. This addresses the findbugs warning.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 14m 6s trunk passed
          +1 compile 0m 19s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 21s trunk passed
          +1 findbugs 0m 28s trunk passed
          +1 javadoc 0m 13s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 18s the patch passed
          +1 compile 0m 17s the patch passed
          +1 javac 0m 17s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 27 unchanged - 0 fixed = 28 total (was 27)
          +1 mvnsite 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 33s the patch passed
          +1 javadoc 0m 12s the patch passed
                Other Tests
          +1 unit 2m 3s hadoop-azure in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          21m 18s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14722
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879977/HADOOP-14722-002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4d22356774aa 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f9139ac
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 14m 6s trunk passed +1 compile 0m 19s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 21s trunk passed +1 findbugs 0m 28s trunk passed +1 javadoc 0m 13s trunk passed       Patch Compile Tests +1 mvninstall 0m 18s the patch passed +1 compile 0m 17s the patch passed +1 javac 0m 17s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 1 new + 27 unchanged - 0 fixed = 28 total (was 27) +1 mvnsite 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 33s the patch passed +1 javadoc 0m 12s the patch passed       Other Tests +1 unit 2m 3s hadoop-azure in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 21m 18s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14722 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879977/HADOOP-14722-002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4d22356774aa 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f9139ac Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12928/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          shanem Shane Mainali added a comment -

          Thanks Thomas Marquardt!

          Does the test cover all of the seek and read scenarios with the stream internal buffer? It'll also be good to add some comments in the test you added. Assuming we are covered in the tests, +1 here from my side.

          Show
          shanem Shane Mainali added a comment - Thanks Thomas Marquardt ! Does the test cover all of the seek and read scenarios with the stream internal buffer? It'll also be good to add some comments in the test you added. Assuming we are covered in the tests, +1 here from my side.
          Hide
          esmanii Esfandiar Manii added a comment - - edited

          BlockBlobInputStream.java: L92-94: streamPosition - streamBufferLength + streamBufferPosition, can this become negative?
          BlockBlobInputStream.java: L133: don't we need to nullify streamBuffer too?
          BlockBlobInputStream.java: L321-323: Why dont you throw the exception right at the beginning?
          BlockBlobInputStream.java: L314: Overall I am not a big fan of having nested if and elses because its making code more complicated that needed. lets just return instead of creating else.
          For example
          public synchronized long skip(long n) throws IOException {
          checkState();
          long skipped;
          if (blobInputStream != null)

          { skipped = blobInputStream.skip(n); streamPosition += skipped; return skipped; }

          if (n < 0 || n > streamLength - streamPosition)

          { throw new IndexOutOfBoundsException("skip range"); }

          if (streamBuffer == null)

          { streamPosition += n; return n; }

          if (n < streamBufferLength - streamBufferPosition)

          { streamBufferPosition += (int) n; }

          else

          { streamBufferPosition = 0; streamBufferLength = 0; streamPosition = getPos() + n; }

          return skipped;
          }

          BlockBlobInputStream.java: L330: I'd suggest create a private method which clears the buffer and get rid of all the custom streamBufferPosition = 0; streamBufferLength = 0 and etc.

          Show
          esmanii Esfandiar Manii added a comment - - edited BlockBlobInputStream.java: L92-94: streamPosition - streamBufferLength + streamBufferPosition, can this become negative? BlockBlobInputStream.java: L133: don't we need to nullify streamBuffer too? BlockBlobInputStream.java: L321-323: Why dont you throw the exception right at the beginning? BlockBlobInputStream.java: L314: Overall I am not a big fan of having nested if and elses because its making code more complicated that needed. lets just return instead of creating else. For example public synchronized long skip(long n) throws IOException { checkState(); long skipped; if (blobInputStream != null) { skipped = blobInputStream.skip(n); streamPosition += skipped; return skipped; } if (n < 0 || n > streamLength - streamPosition) { throw new IndexOutOfBoundsException("skip range"); } if (streamBuffer == null) { streamPosition += n; return n; } if (n < streamBufferLength - streamBufferPosition) { streamBufferPosition += (int) n; } else { streamBufferPosition = 0; streamBufferLength = 0; streamPosition = getPos() + n; } return skipped; } BlockBlobInputStream.java: L330: I'd suggest create a private method which clears the buffer and get rid of all the custom streamBufferPosition = 0; streamBufferLength = 0 and etc.
          Hide
          tmarquardt Thomas Marquardt added a comment -

          Attaching patch HADOOP-14722-003.patch.

          This addresses the feedback, fixes an issue with the BlockBlobInputStream.skip implementation, and adds additional test coverage so all the code paths are exercised for seek and skip.

          All hadoop-tools/hadoop-azure tests are passing with this patch, except for TestWasbRemoteCallHelper which is a known issue tracked by HADOOP-14715. I tested against my endpoint tmarql3.

          Show
          tmarquardt Thomas Marquardt added a comment - Attaching patch HADOOP-14722 -003.patch. This addresses the feedback, fixes an issue with the BlockBlobInputStream.skip implementation, and adds additional test coverage so all the code paths are exercised for seek and skip. All hadoop-tools/hadoop-azure tests are passing with this patch, except for TestWasbRemoteCallHelper which is a known issue tracked by HADOOP-14715 . I tested against my endpoint tmarql3.
          Hide
          tmarquardt Thomas Marquardt added a comment -

          Let me also add the following responses to Esfandiar's feedback:

          >>> BlockBlobInputStream.java: L92-94: streamPosition - streamBufferLength + streamBufferPosition, can this become negative?

          It cannot be negative.

          >>> BlockBlobInputStream.java: L133: don't we need to nullify streamBuffer too?

          The buffer is reusable, to avoid frequent memory allocations of a large buffer. I added a resetStreamBuffer function to set the position and length to zero, to help clarify.

          >>> BlockBlobInputStream.java: L321-323: Why dont you throw the exception right at the beginning?

          A goal of this change was to keep the existing blob input stream functionality up until a reverse seek operation is performed, at which point it switches to the new behavior. The exception was not thrown at the beginning to reduce the likelihood of regression.

          >>> BlockBlobInputStream.java: L314: Overall I am not a big fan of having nested if and elses because its making code more complicated that needed. lets just return instead of creating else.

          I agree, although for this bug fix it is desirable to minimize the code change.

          >>> BlockBlobInputStream.java: L330: I'd suggest create a private method which clears the buffer and get rid of all the custom streamBufferPosition = 0; streamBufferLength = 0 and etc.

          I added the resetStreamBuffer function for this. See my earlier comment about re-using the buffer.

          Show
          tmarquardt Thomas Marquardt added a comment - Let me also add the following responses to Esfandiar's feedback: >>> BlockBlobInputStream.java: L92-94: streamPosition - streamBufferLength + streamBufferPosition, can this become negative? It cannot be negative. >>> BlockBlobInputStream.java: L133: don't we need to nullify streamBuffer too? The buffer is reusable, to avoid frequent memory allocations of a large buffer. I added a resetStreamBuffer function to set the position and length to zero, to help clarify. >>> BlockBlobInputStream.java: L321-323: Why dont you throw the exception right at the beginning? A goal of this change was to keep the existing blob input stream functionality up until a reverse seek operation is performed, at which point it switches to the new behavior. The exception was not thrown at the beginning to reduce the likelihood of regression. >>> BlockBlobInputStream.java: L314: Overall I am not a big fan of having nested if and elses because its making code more complicated that needed. lets just return instead of creating else. I agree, although for this bug fix it is desirable to minimize the code change. >>> BlockBlobInputStream.java: L330: I'd suggest create a private method which clears the buffer and get rid of all the custom streamBufferPosition = 0; streamBufferLength = 0 and etc. I added the resetStreamBuffer function for this. See my earlier comment about re-using the buffer.
          Hide
          shanem Shane Mainali added a comment -

          Thanks Thomas Marquardt for taking care of feedback and adding the additional tests, it's good they caught something. I think this change is in a good state now, +1 from my side.

          Show
          shanem Shane Mainali added a comment - Thanks Thomas Marquardt for taking care of feedback and adding the additional tests, it's good they caught something. I think this change is in a good state now, +1 from my side.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 14m 39s trunk passed
          +1 compile 0m 18s trunk passed
          +1 checkstyle 0m 11s trunk passed
          +1 mvnsite 0m 20s trunk passed
          +1 findbugs 0m 31s trunk passed
          +1 javadoc 0m 13s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 18s the patch passed
          +1 compile 0m 18s the patch passed
          +1 javac 0m 18s the patch passed
          -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 2 new + 27 unchanged - 0 fixed = 29 total (was 27)
          +1 mvnsite 0m 21s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 39s the patch passed
          +1 javadoc 0m 12s the patch passed
                Other Tests
          +1 unit 2m 15s hadoop-azure in the patch passed.
          +1 asflicense 0m 13s The patch does not generate ASF License warnings.
          22m 6s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14722
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880328/HADOOP-14722-003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 37a836c57f0a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f4c6b00
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 14m 39s trunk passed +1 compile 0m 18s trunk passed +1 checkstyle 0m 11s trunk passed +1 mvnsite 0m 20s trunk passed +1 findbugs 0m 31s trunk passed +1 javadoc 0m 13s trunk passed       Patch Compile Tests +1 mvninstall 0m 18s the patch passed +1 compile 0m 18s the patch passed +1 javac 0m 18s the patch passed -0 checkstyle 0m 11s hadoop-tools/hadoop-azure: The patch generated 2 new + 27 unchanged - 0 fixed = 29 total (was 27) +1 mvnsite 0m 21s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 39s the patch passed +1 javadoc 0m 12s the patch passed       Other Tests +1 unit 2m 15s hadoop-azure in the patch passed. +1 asflicense 0m 13s The patch does not generate ASF License warnings. 22m 6s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14722 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880328/HADOOP-14722-003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 37a836c57f0a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f4c6b00 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12950/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          esmanii Esfandiar Manii added a comment -

          +1 On my side too, thanks!

          Show
          esmanii Esfandiar Manii added a comment - +1 On my side too, thanks!
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I think it's OK, though I had a hard time reading through skip(). I've added some comments there.

          I also found it a bit odd reading through seek(), as the value of offset is negative for forward seeks; this is the opposite of other examples S3AInputStream.seekInStream(), SwiftNativeInputStream,seek(). It'd really be good for future maintenance if this class's seek was consistent unless there's a really good reason not to,

          This has just made clear to me we've no FS contract tests for the semantics of InputStream.skip(). I'll do it in the newly created HADOOP-14736 if/when I get round to it.

          Interestingly, InputStream.skip() is pretty vaguely defined; the base class returns 0 for a -ve offset, and again, on any seek past EOF. BlockBlobInputStream isn't following that, but neither does the only implementation of skip() which is tested, the CryptoStream. Nobody has complained so far.

          TestBlockBlobInputStream.test_0201_RandomReadTest

          There's a lot of duplication in this test. Factored it out into a helper method for a bit of clarity.

          Also: noticed the assertEquals() args in test_0200_BasicReadTestV2 were the wrong way round: fixed.

          Overall, I'm happy, as long as you are confident that the tests verify the problem has been fixed. I would like that offset field inverted for consistency with the others (or a justification for not doing so), and a full test run through of my modified patch. I've already tested TestBlockBlobInputStream against Azure Ireland.

          Show
          stevel@apache.org Steve Loughran added a comment - I think it's OK, though I had a hard time reading through skip(). I've added some comments there. I also found it a bit odd reading through seek(), as the value of offset is negative for forward seeks; this is the opposite of other examples S3AInputStream.seekInStream() , SwiftNativeInputStream,seek() . It'd really be good for future maintenance if this class's seek was consistent unless there's a really good reason not to , This has just made clear to me we've no FS contract tests for the semantics of InputStream.skip() . I'll do it in the newly created HADOOP-14736 if/when I get round to it. Interestingly, InputStream.skip() is pretty vaguely defined; the base class returns 0 for a -ve offset, and again, on any seek past EOF. BlockBlobInputStream isn't following that, but neither does the only implementation of skip() which is tested, the CryptoStream. Nobody has complained so far. TestBlockBlobInputStream.test_0201_RandomReadTest There's a lot of duplication in this test. Factored it out into a helper method for a bit of clarity. Also: noticed the assertEquals() args in test_0200_BasicReadTestV2 were the wrong way round: fixed. Overall, I'm happy, as long as you are confident that the tests verify the problem has been fixed. I would like that offset field inverted for consistency with the others (or a justification for not doing so), and a full test run through of my modified patch. I've already tested TestBlockBlobInputStream against Azure Ireland.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 16m 17s trunk passed
          +1 compile 0m 23s trunk passed
          +1 checkstyle 0m 15s trunk passed
          +1 mvnsite 0m 23s trunk passed
          +1 findbugs 0m 34s trunk passed
          +1 javadoc 0m 16s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 19s the patch passed
          +1 compile 0m 21s the patch passed
          +1 javac 0m 21s the patch passed
          -0 checkstyle 0m 12s hadoop-tools/hadoop-azure: The patch generated 2 new + 27 unchanged - 0 fixed = 29 total (was 27)
          +1 mvnsite 0m 21s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 44s the patch passed
          +1 javadoc 0m 14s the patch passed
                Other Tests
          +1 unit 2m 12s hadoop-azure in the patch passed.
          +1 asflicense 0m 14s The patch does not generate ASF License warnings.
          24m 23s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14722
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880469/HADOOP-14722-005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 13fdcda603eb 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f44b349
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 16m 17s trunk passed +1 compile 0m 23s trunk passed +1 checkstyle 0m 15s trunk passed +1 mvnsite 0m 23s trunk passed +1 findbugs 0m 34s trunk passed +1 javadoc 0m 16s trunk passed       Patch Compile Tests +1 mvninstall 0m 19s the patch passed +1 compile 0m 21s the patch passed +1 javac 0m 21s the patch passed -0 checkstyle 0m 12s hadoop-tools/hadoop-azure: The patch generated 2 new + 27 unchanged - 0 fixed = 29 total (was 27) +1 mvnsite 0m 21s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 44s the patch passed +1 javadoc 0m 14s the patch passed       Other Tests +1 unit 2m 12s hadoop-azure in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 24m 23s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14722 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880469/HADOOP-14722-005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 13fdcda603eb 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f44b349 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12961/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          tmarquardt Thomas Marquardt added a comment -

          Attaching HADOOP-14722-006.patch.

          Thanks for the feedback Steve! I took your 005 patch and 1) added additional comments, 2) updated BlockBlobInputStream.seek() so the offset is positive for forward seek and negative for reverse seek, 3) changed the test name BasicReadTestV2 to BasicReadTest since it validates V1 and V2 are identical, and 4) changed the access modifier of TestBlockBlobInputStream.verifyConsistentReads() from protected to private.

          I reviewed the de-duplication change you made to test_0201_RandomReadTest and it looks perfect.

          All hadoop-tools/hadoop-azure tests are passing against my tmarql3 storage account. I set "fs.azure.secure.mode" to false due to the HADOOP-14715 issue we are still working on. Here are the test results:

          Tests run: 774, Failures: 0, Errors: 0, Skipped: 155

          Show
          tmarquardt Thomas Marquardt added a comment - Attaching HADOOP-14722 -006.patch. Thanks for the feedback Steve! I took your 005 patch and 1) added additional comments, 2) updated BlockBlobInputStream.seek() so the offset is positive for forward seek and negative for reverse seek, 3) changed the test name BasicReadTestV2 to BasicReadTest since it validates V1 and V2 are identical, and 4) changed the access modifier of TestBlockBlobInputStream.verifyConsistentReads() from protected to private. I reviewed the de-duplication change you made to test_0201_RandomReadTest and it looks perfect. All hadoop-tools/hadoop-azure tests are passing against my tmarql3 storage account. I set "fs.azure.secure.mode" to false due to the HADOOP-14715 issue we are still working on. Here are the test results: Tests run: 774, Failures: 0, Errors: 0, Skipped: 155
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 17m 6s trunk passed
          +1 compile 0m 24s trunk passed
          +1 checkstyle 0m 16s trunk passed
          +1 mvnsite 0m 26s trunk passed
          +1 findbugs 0m 35s trunk passed
          +1 javadoc 0m 17s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 23s the patch passed
          +1 compile 0m 22s the patch passed
          +1 javac 0m 22s the patch passed
          -0 checkstyle 0m 13s hadoop-tools/hadoop-azure: The patch generated 3 new + 25 unchanged - 2 fixed = 28 total (was 27)
          +1 mvnsite 0m 24s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 43s the patch passed
          +1 javadoc 0m 14s the patch passed
                Other Tests
          +1 unit 2m 19s hadoop-azure in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          25m 38s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HADOOP-14722
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880503/HADOOP-14722-006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4c69aa92d7bd 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 024c3ec
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/testReport/
          modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 17m 6s trunk passed +1 compile 0m 24s trunk passed +1 checkstyle 0m 16s trunk passed +1 mvnsite 0m 26s trunk passed +1 findbugs 0m 35s trunk passed +1 javadoc 0m 17s trunk passed       Patch Compile Tests +1 mvninstall 0m 23s the patch passed +1 compile 0m 22s the patch passed +1 javac 0m 22s the patch passed -0 checkstyle 0m 13s hadoop-tools/hadoop-azure: The patch generated 3 new + 25 unchanged - 2 fixed = 28 total (was 27) +1 mvnsite 0m 24s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 43s the patch passed +1 javadoc 0m 14s the patch passed       Other Tests +1 unit 2m 19s hadoop-azure in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 25m 38s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14722 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880503/HADOOP-14722-006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4c69aa92d7bd 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 024c3ec Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/artifact/patchprocess/diff-checkstyle-hadoop-tools_hadoop-azure.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12963/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          +1
          committed to branch-2 & trunk, with clean build and a run of the new test on each branch to verify.

          Thanks.

          Show
          stevel@apache.org Steve Loughran added a comment - +1 committed to branch-2 & trunk, with clean build and a run of the new test on each branch to verify. Thanks.

            People

            • Assignee:
              tmarquardt Thomas Marquardt
              Reporter:
              tmarquardt Thomas Marquardt
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:
                Resolved:

                Development