Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-727

bug setting block size hdfsOpenFile

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.20.2
    • Fix Version/s: 0.20.3, 0.22.0
    • Component/s: libhdfs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In hdfsOpenFile in libhdfs invokeMethod needs to cast the block size argument to a jlong so a full 8 bytes are passed (rather than 4 plus some garbage which causes writes to fail due to a bogus block size).

      1. hdfs727.patch
        1 kB
        Eli Collins
      2. hdfs727.patch
        2 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          This patch fixes the above bug, nukes an unused define, and makes a minor fix to the declaration of classname in constructNewArrayString (which running with -Xcheck:jni discovered).

          Show
          Eli Collins added a comment - This patch fixes the above bug, nukes an unused define, and makes a minor fix to the declaration of classname in constructNewArrayString (which running with -Xcheck:jni discovered).
          Hide
          dhruba borthakur added a comment -

          does this occur on a [articular OS platform? solaris? redhat linux?

          Show
          dhruba borthakur added a comment - does this occur on a [articular OS platform? solaris? redhat linux?
          Hide
          Eli Collins added a comment -

          I saw this bug on 64-bit linux, but you could see it on any 64-bit system.

          invokeMethod(env, &jVal, &jExc, INSTANCE, jConfiguration, 
                                        HADOOP_CONF, "getLong", "(Ljava/lang/String;J)J",
                                       jStrBlockSize, 67108864))  {
          

          In the above code the value 67108864 will be passed as an int since the literal fits in an int (the compiler won't do type promotion here because it doesn't know argument types for variadic functions). Meanwhile invokeMethod passes the va_list to CallObjectMethodV which expects a jlong (8 bytes). The behavior is undefined, in practice it read 8 bytes when accessing this parameter, which ends up being the 4 byte value plus 4 bytes of whatever was passed next to it.

          Show
          Eli Collins added a comment - I saw this bug on 64-bit linux, but you could see it on any 64-bit system. invokeMethod(env, &jVal, &jExc, INSTANCE, jConfiguration, HADOOP_CONF, "getLong" , "(Ljava/lang/ String ;J)J" , jStrBlockSize, 67108864)) { In the above code the value 67108864 will be passed as an int since the literal fits in an int (the compiler won't do type promotion here because it doesn't know argument types for variadic functions). Meanwhile invokeMethod passes the va_list to CallObjectMethodV which expects a jlong (8 bytes). The behavior is undefined, in practice it read 8 bytes when accessing this parameter, which ends up being the 4 byte value plus 4 bytes of whatever was passed next to it.
          Hide
          dhruba borthakur added a comment -

          This seems to be a serious problem. I would like to backport it to 0.20.2 and 0.21. Can we enhannce hdfs_test.c to include a unit test to test this patch?

          Show
          dhruba borthakur added a comment - This seems to be a serious problem. I would like to backport it to 0.20.2 and 0.21. Can we enhannce hdfs_test.c to include a unit test to test this patch?
          Hide
          Eli Collins added a comment -

          hdfs_test doesn't catch this, I confirmed hdfsOpenFile as used in the test does set the block size incorrectly but I suspect the create path isn't going through the BlockManager (which would fail the write because no DN matches the requested block size). I'll update the test to catch this bug.

          Show
          Eli Collins added a comment - hdfs_test doesn't catch this, I confirmed hdfsOpenFile as used in the test does set the block size incorrectly but I suspect the create path isn't going through the BlockManager (which would fail the write because no DN matches the requested block size). I'll update the test to catch this bug.
          Hide
          dhruba borthakur added a comment -

          Hi Eli, Thanks for offering to update the test case, will wait for that one. This is something that is worth putting into the 0.20.3 release.

          Show
          dhruba borthakur added a comment - Hi Eli, Thanks for offering to update the test case, will wait for that one. This is something that is worth putting into the 0.20.3 release.
          Hide
          Eli Collins added a comment -

          Hey Dhruba,

          Now that I can run the libhdfs test on trunk (HDFS-756), I ran the libhdfs test w/o the patch in this jira and confirmed that on an ubuntu 9.10 64-bit host the test fails due to this bug. Adding
          fprintf(stderr, "jBlockSize=%lld\n", jBlockSize); in hdfsOpenFile shows the corrupt value in test output, and the failure (could only be replicated to 0 nodes) is the same failure I saw before (there's no node that will accept this large block size).

          [exec] jBlockSize 47403621154816
          ...
          [exec] 09/11/16 20:08:06 WARN hdfs.DFSClient: DataStreamer Exception: org.apache.hadoop.ipc.RemoteException: java.io.IOException:
          File /tmp/testfile.txt could only be replicated to 0 nodes, instead of 1

          The patch still applies against trunk and 20.1 and 20.2.

          Thanks,
          Eli

          Show
          Eli Collins added a comment - Hey Dhruba, Now that I can run the libhdfs test on trunk ( HDFS-756 ), I ran the libhdfs test w/o the patch in this jira and confirmed that on an ubuntu 9.10 64-bit host the test fails due to this bug. Adding fprintf(stderr, "jBlockSize=%lld\n", jBlockSize); in hdfsOpenFile shows the corrupt value in test output, and the failure (could only be replicated to 0 nodes) is the same failure I saw before (there's no node that will accept this large block size). [exec] jBlockSize 47403621154816 ... [exec] 09/11/16 20:08:06 WARN hdfs.DFSClient: DataStreamer Exception: org.apache.hadoop.ipc.RemoteException: java.io.IOException: File /tmp/testfile.txt could only be replicated to 0 nodes, instead of 1 The patch still applies against trunk and 20.1 and 20.2. Thanks, Eli
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422956/hdfs727.patch
          against trunk revision 881017.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/77/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422956/hdfs727.patch against trunk revision 881017. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/77/console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Looks like the diff to hdfsJniHelper.h was already applied, here's the patch again w/o that diff.

          Show
          Eli Collins added a comment - Looks like the diff to hdfsJniHelper.h was already applied, here's the patch again w/o that diff.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12425186/hdfs727.patch
          against trunk revision 881695.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          -1 release audit. The applied patch generated 126 release audit warnings (more than the trunk's current 0 warnings).

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12425186/hdfs727.patch against trunk revision 881695. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 126 release audit warnings (more than the trunk's current 0 warnings). -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/78/console This message is automatically generated.
          Hide
          Konstantin Boudnik added a comment -

          +1 patch looks good.

          I've verified if it works for HDFS-756 and it seems to be doing the job. I'm going to wait till tomorrow if anyone wants to comment on this and will commit it.

          Show
          Konstantin Boudnik added a comment - +1 patch looks good. I've verified if it works for HDFS-756 and it seems to be doing the job. I'm going to wait till tomorrow if anyone wants to comment on this and will commit it.
          Hide
          Konstantin Boudnik added a comment -

          Clearly, the problem with the test is unrelated. However, I have executed the failing test case from test-patch run with this patch in place and test test is passing ok.

          Show
          Konstantin Boudnik added a comment - Clearly, the problem with the test is unrelated. However, I have executed the failing test case from test-patch run with this patch in place and test test is passing ok.
          Hide
          Konstantin Boudnik added a comment -

          This issue doesn't apply to 0.21 for there are needed sources available.

          Show
          Konstantin Boudnik added a comment - This issue doesn't apply to 0.21 for there are needed sources available.
          Hide
          Konstantin Boudnik added a comment -

          I've just committed this. Thanks Eli.

          Show
          Konstantin Boudnik added a comment - I've just committed this. Thanks Eli.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #117 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/117/)
          . bug setting block size hdfsOpenFile. Contributed by Eli Collins.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #117 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/117/ ) . bug setting block size hdfsOpenFile. Contributed by Eli Collins.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #121 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/121/)
          . bug setting block size hdfsOpenFile. Contributed by Eli Collins.

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #121 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/121/ ) . bug setting block size hdfsOpenFile. Contributed by Eli Collins.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #150 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/150/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #150 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/150/ )
          Hide
          Eli Collins added a comment -

          It doesn't look like the change made it into branch-0.20. Cos, can you check it in there?

          Show
          Eli Collins added a comment - It doesn't look like the change made it into branch-0.20. Cos, can you check it in there?
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #81 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/81/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #81 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/81/ )
          Hide
          Eli Collins added a comment -

          I merged HDFS-727 from trunk into branch-0.20, ran the libhdfs test and committed.

          Show
          Eli Collins added a comment - I merged HDFS-727 from trunk into branch-0.20, ran the libhdfs test and committed.

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development