Hadoop Common
  1. Hadoop Common
  2. HADOOP-4508

FSDataOutputStream.getPos() == 0when appending to existing file and should be file length

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.19.0, 0.20.0
    • Fix Version/s: 0.19.1
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      does not seem getPos is set correctly when opening an existing file with non-zero length in append mode.

      1. appendGetPos.patch
        4 kB
        dhruba borthakur
      2. appendGetPos2.patch
        4 kB
        dhruba borthakur
      3. appendGetPos3.patch
        4 kB
        dhruba borthakur
      4. HADOOP-4508.txt
        0.7 kB
        Pete Wyckoff

        Issue Links

          Activity

          Hide
          Pete Wyckoff added a comment -

          attaching a diff for TestAppend2::testSimpleAppend that demonstrates the issue with a simple assertTrue(stm.getPos() > 0);

          Show
          Pete Wyckoff added a comment - attaching a diff for TestAppend2::testSimpleAppend that demonstrates the issue with a simple assertTrue(stm.getPos() > 0);
          Hide
          Konstantin Shvachko added a comment -

          Dhruba, can we do something to avoid returning FileStatus as an array of size one.
          You just need the number of block out of it right?

          Show
          Konstantin Shvachko added a comment - Dhruba, can we do something to avoid returning FileStatus as an array of size one. You just need the number of block out of it right?
          Hide
          dhruba borthakur added a comment -

          Yes, we should avoid returning an array of size 1. That part was mostly a hack to determine if this was this works.

          We need to the size of the file returned to the caller.

          Show
          dhruba borthakur added a comment - Yes, we should avoid returning an array of size 1. That part was mostly a hack to determine if this was this works. We need to the size of the file returned to the caller.
          Hide
          Pete Wyckoff added a comment -

          applied the patch and ran libhdfs and fuse-dfs unit tests and all passed!!

          Show
          Pete Wyckoff added a comment - applied the patch and ran libhdfs and fuse-dfs unit tests and all passed!!
          Hide
          dhruba borthakur added a comment -

          Incorporated Konstantin's review comments.

          Show
          dhruba borthakur added a comment - Incorporated Konstantin's review comments.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12393206/appendGetPos2.patch
          against trunk revision 709609.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

          +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 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/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/12393206/appendGetPos2.patch against trunk revision 709609. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3520/console This message is automatically generated.
          Hide
          dhruba borthakur added a comment -

          Hi Konstantin, it would be nice if you could review this one. Thanks.

          Show
          dhruba borthakur added a comment - Hi Konstantin, it would be nice if you could review this one. Thanks.
          Hide
          dhruba borthakur added a comment -

          can some hdfs contributor pl review this one?

          Show
          dhruba borthakur added a comment - can some hdfs contributor pl review this one?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          How about make initialFileSize final and then getInitialLen() don't need to be synchronized?

          Show
          Tsz Wo Nicholas Sze added a comment - How about make initialFileSize final and then getInitialLen() don't need to be synchronized?
          Hide
          dhruba borthakur added a comment -

          Work on Nicholas's review comments.

          Show
          dhruba borthakur added a comment - Work on Nicholas's review comments.
          Hide
          dhruba borthakur added a comment -

          Removed the "synchronized" on getInitialLength() because the initialFileSize is initialized in the constructor itself. (I am not finding i very elegant to make it final because it is set inside one constructor invokes the other constructor)

          Show
          dhruba borthakur added a comment - Removed the "synchronized" on getInitialLength() because the initialFileSize is initialized in the constructor itself. (I am not finding i very elegant to make it final because it is set inside one constructor invokes the other constructor)
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > I am not finding i very elegant to make it final because it is set inside one constructor invokes the other constructor

          yeah, you need to change all constructors to make it work. I am fine with the current code.

          +1 new patch looks good

          Show
          Tsz Wo Nicholas Sze added a comment - > I am not finding i very elegant to make it final because it is set inside one constructor invokes the other constructor yeah, you need to change all constructors to make it work. I am fine with the current code. +1 new patch looks good
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12393858/appendGetPos3.patch
          against trunk revision 713893.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

          +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 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/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/12393858/appendGetPos3.patch against trunk revision 713893. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3589/console This message is automatically generated.
          Hide
          dhruba borthakur added a comment -

          I would like this to go into 0.19 branch.

          Show
          dhruba borthakur added a comment - I would like this to go into 0.19 branch.
          Hide
          Pete Wyckoff added a comment -

          any update on this? looks ready to commit to 20 and 19.1. I think as dhruba said it really is a blocker since it is a bad bug when opening a file in append mode.

          thanks, pete

          Show
          Pete Wyckoff added a comment - any update on this? looks ready to commit to 20 and 19.1. I think as dhruba said it really is a blocker since it is a bad bug when opening a file in append mode. thanks, pete
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I just committed this. Thanks, Dhruba!

          Show
          Tsz Wo Nicholas Sze added a comment - I just committed this. Thanks, Dhruba!
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #680 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/680/ )

            People

            • Assignee:
              dhruba borthakur
              Reporter:
              Pete Wyckoff
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development