Hadoop Common
  1. Hadoop Common
  2. HADOOP-5333

The libhdfs append API is not coded correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.19.2
    • Component/s: None
    • Labels:
      None

      Description

      The hdfsOpenFile() API does not handle the APPEND bit correctly.

      1. libhdfsAppend.patch
        5 kB
        dhruba borthakur
      2. libhdfsAppend.patch
        3 kB
        dhruba borthakur
      3. libhdfsAppend.patch
        2 kB
        dhruba borthakur

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #778 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/778/ )
        Hide
        dhruba borthakur added a comment -

        I just committed this.

        Show
        dhruba borthakur added a comment - I just committed this.
        Hide
        dhruba borthakur added a comment -

        The patch changes libhdfs only but the failed unit test was in core mapred package. This failure is not related to this patch.

        Show
        dhruba borthakur added a comment - The patch changes libhdfs only but the failed unit test was in core mapred package. This failure is not related to this patch.
        Hide
        Hadoop QA added a comment -

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

        +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 release audit. The applied patch does not increase the total number of release audit 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/Hadoop-Patch-minerva.apache.org/44/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/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/12401789/libhdfsAppend.patch against trunk revision 752073. +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 release audit. The applied patch does not increase the total number of release audit 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/Hadoop-Patch-minerva.apache.org/44/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/44/console This message is automatically generated.
        Hide
        Chris Douglas added a comment -

        +1

        Show
        Chris Douglas added a comment - +1
        Hide
        dhruba borthakur added a comment -

        Create a separate lock for JVM initialization.

        Show
        dhruba borthakur added a comment - Create a separate lock for JVM initialization.
        Hide
        dhruba borthakur added a comment -

        Apply review comments.

        Show
        dhruba borthakur added a comment - Apply review comments.
        Hide
        Chris Douglas added a comment -

        But since libhdfs is a library and there are no "main" methods, can you suggest which "initialization" method might create the JVM?

        I can't. I don't know enough about libhdfs, nor do I know any clever ways to trap init generally.

        I can create a different lock solely for this initialization

        That seems reasonable. If there were other globals to be initialized, adding the missing hdfs_init (or whatever) might make sense, but a separate lock is probably sufficient.

        Show
        Chris Douglas added a comment - But since libhdfs is a library and there are no "main" methods, can you suggest which "initialization" method might create the JVM? I can't. I don't know enough about libhdfs, nor do I know any clever ways to trap init generally. I can create a different lock solely for this initialization That seems reasonable. If there were other globals to be initialized, adding the missing hdfs_init (or whatever) might make sense, but a separate lock is probably sufficient.
        Hide
        dhruba borthakur added a comment -

        Thanks a bunch Chris for reviewing this patch.

        Only one JVM is needed. It would be nice to create the JVM during initialization rather than lazily in getJNIEnv(). But since libhdfs is a library and there are no "main" methods, can you suggest which "initialization" method might create the JVM?

        I can create a different lock solely for this initialization (instead of using LOCK_HASH_TABLE).

        Show
        dhruba borthakur added a comment - Thanks a bunch Chris for reviewing this patch. Only one JVM is needed. It would be nice to create the JVM during initialization rather than lazily in getJNIEnv(). But since libhdfs is a library and there are no "main" methods, can you suggest which "initialization" method might create the JVM? I can create a different lock solely for this initialization (instead of using LOCK_HASH_TABLE).
        Hide
        Chris Douglas added a comment -

        sigh Good catch on the braces.

        I'm not sure I follow the locking added to getJNIEnv. Is it related to this issue? If only one JVM should be created and shared between all threads, can that be part of init and not lazily created? It looks like most/all paths through libhdfs require that it exist... is it handling failures by relying on get/set, i.e. when/if the JVM dies? I'm not very familiar with libhdfs so please correct me if I've misread this, but appropriating the lock guarding updates to a cache of class objects to protect the creation of a singleton seems to regard locks as desperately scarce resources. If it's difficult to do otherwise, then perhaps this can be a separate issue...

        Show
        Chris Douglas added a comment - sigh Good catch on the braces. I'm not sure I follow the locking added to getJNIEnv. Is it related to this issue? If only one JVM should be created and shared between all threads, can that be part of init and not lazily created? It looks like most/all paths through libhdfs require that it exist... is it handling failures by relying on get/set, i.e. when/if the JVM dies? I'm not very familiar with libhdfs so please correct me if I've misread this, but appropriating the lock guarding updates to a cache of class objects to protect the creation of a singleton seems to regard locks as desperately scarce resources. If it's difficult to do otherwise, then perhaps this can be a separate issue...
        Hide
        dhruba borthakur added a comment -

        The test failed somewhere in the mapreduce package whereas this patch changes libhdfs. This patch cannot cause these test failures.

        Show
        dhruba borthakur added a comment - The test failed somewhere in the mapreduce package whereas this patch changes libhdfs. This patch cannot cause these test failures.
        Hide
        Hadoop QA added a comment -

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

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

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/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/12401197/libhdfsAppend.patch against trunk revision 748861. +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 tests are needed for 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 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/25/console This message is automatically generated.
        Hide
        dhruba borthakur added a comment -

        There should be a lock around getJniENV() to ensure that only one thread creates the JVM and the other threads use the same JVM. I am using the hashtable lock to protect the creation of the JVM.

        Show
        dhruba borthakur added a comment - There should be a lock around getJniENV() to ensure that only one thread creates the JVM and the other threads use the same JVM. I am using the hashtable lock to protect the creation of the JVM.
        Hide
        dhruba borthakur added a comment -

        Can somebody please review this patch?

        Show
        dhruba borthakur added a comment - Can somebody please review this patch?
        Hide
        dhruba borthakur added a comment -

        Arrange the curly-braces in the code appropriately so that the FileSystem.append method gets invoked correctly.

        Show
        dhruba borthakur added a comment - Arrange the curly-braces in the code appropriately so that the FileSystem.append method gets invoked correctly.

          People

          • Assignee:
            dhruba borthakur
            Reporter:
            dhruba borthakur
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development