Details

    • Type: Test Test
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: fuse-dfs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Looks like TestFuseDFS has bit rot. Let's revive it.

      1. hdfs-780-1.patch
        11 kB
        Eli Collins
      2. hdfs-780-2.patch
        38 kB
        Eli Collins
      3. hdfs-780-3.patch
        37 kB
        Eli Collins
      4. hdfs-780-4.patch
        69 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Eli Collins added a comment -

          This class needs some more tests to cover the following jiras:

          Show
          Eli Collins added a comment - This class needs some more tests to cover the following jiras: Return codes ( HDFS-858 ) Truncate with zero and non-zero lengths ( HDFS-860 ) Test testUtimes should test directories ( HDFS-859 ) Test rdwr ( HDFS-861 )
          Hide
          Eli Collins added a comment -

          Attached a patch that fixes up all the build files to get the test running again. The test itself fails due to HDFS-940 and some issues with the java code in the test itself. Run with:

          $ ant -Dcompile.c++=true -Dlibhdfs=true compile
          $ ant -Dlibhdfs=1 -Dfusedfs=1 test-contrib 
          
          Show
          Eli Collins added a comment - Attached a patch that fixes up all the build files to get the test running again. The test itself fails due to HDFS-940 and some issues with the java code in the test itself. Run with: $ ant -Dcompile.c++= true -Dlibhdfs= true compile $ ant -Dlibhdfs=1 -Dfusedfs=1 test-contrib
          Hide
          Eli Collins added a comment -

          New patch attached. This one also gets the tests running and rewrites them.

          Show
          Eli Collins added a comment - New patch attached. This one also gets the tests running and rewrites them.
          Hide
          Eli Collins added a comment -

          Minor update to the last patch: adds fuse-dfs back into build-contrib.xml so it runs as part of test-contrib but the targets only run if the fuse-dfs argument is passed.

          Show
          Eli Collins added a comment - Minor update to the last patch: adds fuse-dfs back into build-contrib.xml so it runs as part of test-contrib but the targets only run if the fuse-dfs argument is passed.
          Hide
          Todd Lipcon added a comment -
          • why use <exec executable="cp"...> instead of using the built-in ant copy task?
          • not clear why you have to redefine test.classpath - is the one that comes from build-contrib.xml not sufficient?
          • why set fs.default.name property in build.xml? This becomes a Java system property which afaik would be entirely ignored?
          • I noticed AM_CPPFLAGS has -D_FILE_OFFSET_BITS=64, but seems to me this should be done with autoconf AC_SYS_LARGEFILE. Probably out of scope for this review though.
          • spelling: "succesfully"
          • javadoc for execAssertFalse is incorrect
          • might be clearer to call those methods assertExecSucceeds and assertExecFails?
          • what's the purpose of the 1 second sleep in createFile? there's some kind of delay between making a file and it being visible or something? this seems like a suprising semantic and at least deserves a comment
          • in checkFile, maybe need a try...finally to avoid leaving file open on IOE?
          • in checkFile, you assume that read won't return less than the whole file. Maybe you could use IOUtil.copyBytes() into a ByteArrayOutputStream?
          • establishMount seems to assume libhdfs is installed in /usr/local/lib? Also uses the outdated dfs:// URI type instead of hdfs://
          • establishMount doesn't seem to check exit code of the mount command?
          Show
          Todd Lipcon added a comment - why use <exec executable="cp"...> instead of using the built-in ant copy task? not clear why you have to redefine test.classpath - is the one that comes from build-contrib.xml not sufficient? why set fs.default.name property in build.xml? This becomes a Java system property which afaik would be entirely ignored? I noticed AM_CPPFLAGS has -D_FILE_OFFSET_BITS=64, but seems to me this should be done with autoconf AC_SYS_LARGEFILE. Probably out of scope for this review though. spelling: "succesfully" javadoc for execAssertFalse is incorrect might be clearer to call those methods assertExecSucceeds and assertExecFails? what's the purpose of the 1 second sleep in createFile? there's some kind of delay between making a file and it being visible or something? this seems like a suprising semantic and at least deserves a comment in checkFile, maybe need a try...finally to avoid leaving file open on IOE? in checkFile, you assume that read won't return less than the whole file. Maybe you could use IOUtil.copyBytes() into a ByteArrayOutputStream? establishMount seems to assume libhdfs is installed in /usr/local/lib? Also uses the outdated dfs:// URI type instead of hdfs:// establishMount doesn't seem to check exit code of the mount command?
          Hide
          Eli Collins added a comment -

          Thanks Todd! Comments follow.

          why use <exec executable="cp"...> instead of using the built-in ant copy task?

          The copy task doesn't preserve the exec bit. See http://ant.apache.org/manual/Tasks/copy.html
          Added a comment.

          not clear why you have to redefine test.classpath - is the one that comes from build-contrib.xml not sufficient?

          Not necessary, removed it. (not sure why I had it, did the first cut of this patch a while back).

          why set fs.default.name property in build.xml? This becomes a Java system property which afaik would be entirely ignored?

          Ditto.

          I noticed AM_CPPFLAGS has -D_FILE_OFFSET_BITS=64, but seems to me this should be done with autoconf AC_SYS_LARGEFILE. Probably out of scope for this review though.

          For some reason adding AC_PROG_CC and AC_SYS_LARGEFILE to configure.ac was insufficient. Still needed FILE_OFFSET_BITS, will punt on cleaning up the build further to a future change.

          spelling: "succesfully"

          Fixed.

          javadoc for execAssertFalse is incorrect

          Fixed.

          might be clearer to call those methods assertExecSucceeds and assertExecFails?

          Agree, done.

          what's the purpose of the 1 second sleep in createFile? there's

          some kind of delay between making a file and it being visible or
          something? this seems like a suprising semantic and at least deserves
          a comment

          That was earlier debugging, however I just removed it and found that if there is not a sleep after createFile in testCreate the call to checkFile fails with a premature EOF. I'll file a bug to investigate further.

          in checkFile, maybe need a try...finally to avoid leaving file open on IOE?

          Done.

          in checkFile, you assume that read won't return less than the

          whole file. Maybe you could use IOUtil.copyBytes() into a
          ByteArrayOutputStream?

          Switched to IOUtil#readFully (ditto for creating the file).

          establishMount seems to assume libhdfs is installed in /usr/local/lib? Also uses the outdated dfs:// URI type instead of hdfs://

          Leftover from the previous version, fixed. fuse-dfs actually doesn't support "hdfs" in the URI, will fix in a separate change.

          establishMount doesn't seem to check exit code of the mount command?

          This is because the fuse-dfs process is run in debug mode (not daemonized) and it's stderr and out are captured. On second thought it's better to run daemonized and just capture syslog. Incidentally the hang in the last test was due to the test blocking on fuse-dfs and fuse-dfs blocking on the test to read it's output. Fixed now.

          To implement this last part I needed to make sure all useful debug info went to go both to stderr and syslog. I've introduced macros that do this and modified all the fuse-dfs code to use them.

          Btw I also removed bootstraph.sh and made the build file execute this (as part of the compile target instead of the test target).

          I also added a test that covers access via multiple threads.

          I've run the tests on 64-bit Ubuntu Maverick and Centos 5 hosts.

          Show
          Eli Collins added a comment - Thanks Todd! Comments follow. why use <exec executable="cp"...> instead of using the built-in ant copy task? The copy task doesn't preserve the exec bit. See http://ant.apache.org/manual/Tasks/copy.html Added a comment. not clear why you have to redefine test.classpath - is the one that comes from build-contrib.xml not sufficient? Not necessary, removed it. (not sure why I had it, did the first cut of this patch a while back). why set fs.default.name property in build.xml? This becomes a Java system property which afaik would be entirely ignored? Ditto. I noticed AM_CPPFLAGS has -D_FILE_OFFSET_BITS=64, but seems to me this should be done with autoconf AC_SYS_LARGEFILE. Probably out of scope for this review though. For some reason adding AC_PROG_CC and AC_SYS_LARGEFILE to configure.ac was insufficient. Still needed FILE_OFFSET_BITS, will punt on cleaning up the build further to a future change. spelling: "succesfully" Fixed. javadoc for execAssertFalse is incorrect Fixed. might be clearer to call those methods assertExecSucceeds and assertExecFails? Agree, done. what's the purpose of the 1 second sleep in createFile? there's some kind of delay between making a file and it being visible or something? this seems like a suprising semantic and at least deserves a comment That was earlier debugging, however I just removed it and found that if there is not a sleep after createFile in testCreate the call to checkFile fails with a premature EOF. I'll file a bug to investigate further. in checkFile, maybe need a try...finally to avoid leaving file open on IOE? Done. in checkFile, you assume that read won't return less than the whole file. Maybe you could use IOUtil.copyBytes() into a ByteArrayOutputStream? Switched to IOUtil#readFully (ditto for creating the file). establishMount seems to assume libhdfs is installed in /usr/local/lib? Also uses the outdated dfs:// URI type instead of hdfs:// Leftover from the previous version, fixed. fuse-dfs actually doesn't support "hdfs" in the URI, will fix in a separate change. establishMount doesn't seem to check exit code of the mount command? This is because the fuse-dfs process is run in debug mode (not daemonized) and it's stderr and out are captured. On second thought it's better to run daemonized and just capture syslog. Incidentally the hang in the last test was due to the test blocking on fuse-dfs and fuse-dfs blocking on the test to read it's output. Fixed now. To implement this last part I needed to make sure all useful debug info went to go both to stderr and syslog. I've introduced macros that do this and modified all the fuse-dfs code to use them. Btw I also removed bootstraph.sh and made the build file execute this (as part of the compile target instead of the test target). I also added a test that covers access via multiple threads. I've run the tests on 64-bit Ubuntu Maverick and Centos 5 hosts.
          Hide
          Eli Collins added a comment -

          Right patch this time.

          Show
          Eli Collins added a comment - Right patch this time.
          Hide
          Todd Lipcon added a comment -

          +1, looks good to me. Could probably refactor the exec stuff to use o.a.h.util.Shell (which handles pumping stderr from a separate thread to avoid the deadlock issue), but since this is test code, doesn't strike me as a big deal.

          Show
          Todd Lipcon added a comment - +1, looks good to me. Could probably refactor the exec stuff to use o.a.h.util.Shell (which handles pumping stderr from a separate thread to avoid the deadlock issue), but since this is test code, doesn't strike me as a big deal.
          Hide
          Eli Collins added a comment -

          Thanks Todd. I've committed this.

          Show
          Eli Collins added a comment - Thanks Todd. I've committed this.
          Hide
          Hudson added a comment -

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

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

          Integrated in Hadoop-Hdfs-trunk #643 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/643/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #643 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/643/ )

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development