Hadoop Common
  1. Hadoop Common
  2. HADOOP-4467

SerializationFactory should use current context ClassLoader

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.18.1, 0.19.0
    • Fix Version/s: 0.18.2
    • Component/s: None
    • Labels:
      None
    • Release Note:
      SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances.

      Description

      SerializationFactory#add calls Class.forName without a ClassLoader instance. This prevents user-space Serialization classes from being loaded.

      The resulting ClassNotFoundException is eaten after being logged, and a NPE is thrown further down the line when a Serializer is requested.

      1. hadoop-4467-v2.patch
        2 kB
        Chris K Wensel
      2. hadoop-4467.patch
        1 kB
        Chris K Wensel

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #640 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/640/)
        . SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances. Contributed by Chris Wensel.

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #640 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/640/ ) . SerializationFactory now uses the current context ClassLoader allowing for user supplied Serialization instances. Contributed by Chris Wensel.
        Hide
        Arun C Murthy added a comment -

        I just committed this. Thanks, Chris!

        Show
        Arun C Murthy added a comment - I just committed this. Thanks, Chris!
        Hide
        Chris K Wensel added a comment -

        via ant test-patch...

        seems to be down on lack of tests. unsure, per previous comment, how to test classloader issues.

        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
        [exec] Please justify why no tests are needed for this patch.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]

        Show
        Chris K Wensel added a comment - via ant test-patch... seems to be down on lack of tests. unsure, per previous comment, how to test classloader issues. [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec]
        Hide
        Chris K Wensel added a comment -

        All but org.apache.hadoop.fs.TestLocalDirAllocator tests pass.

        I think this is unrelated:
        shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory

        Show
        Chris K Wensel added a comment - All but org.apache.hadoop.fs.TestLocalDirAllocator tests pass. I think this is unrelated: shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
        Hide
        Tom White added a comment -

        +1

        Show
        Tom White added a comment - +1
        Hide
        Chris K Wensel added a comment -

        delegates back to Configuration

        Show
        Chris K Wensel added a comment - delegates back to Configuration
        Hide
        Chris K Wensel added a comment -

        delegates classloading back to Configuration

        Show
        Chris K Wensel added a comment - delegates classloading back to Configuration
        Hide
        Chris K Wensel added a comment -

        agreed.

        Show
        Chris K Wensel added a comment - agreed.
        Hide
        Owen O'Malley added a comment -

        Wouldn't it be better to use the class loader associated with the configuration rather than the current thread? Basically, use

           conf.getClassByName(name);
        

        Clearly the conf for the serializer would need to be saved from the constructor.

        Show
        Owen O'Malley added a comment - Wouldn't it be better to use the class loader associated with the configuration rather than the current thread? Basically, use conf.getClassByName(name); Clearly the conf for the serializer would need to be saved from the constructor.
        Hide
        Chris K Wensel added a comment -

        initial attempt, without unit tests

        Show
        Chris K Wensel added a comment - initial attempt, without unit tests
        Hide
        Chris K Wensel added a comment -

        added 0.19 as an affected version

        Show
        Chris K Wensel added a comment - added 0.19 as an affected version
        Hide
        Chris K Wensel added a comment -

        I probably should have phrased this one differently, but it does seem to be standard practice to provide the context classloader when calling Class.forName.

        I can submit a patch with the one line change. but I do not know if there should (or even could be) an accompanying unit test for this. please advise.

        Show
        Chris K Wensel added a comment - I probably should have phrased this one differently, but it does seem to be standard practice to provide the context classloader when calling Class.forName. I can submit a patch with the one line change. but I do not know if there should (or even could be) an accompanying unit test for this. please advise.

          People

          • Assignee:
            Chris K Wensel
            Reporter:
            Chris K Wensel
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development