HBase
  1. HBase
  2. HBASE-7851

Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.94.4
    • Fix Version/s: 0.94.6, 0.95.1
    • Component/s: None
    • Labels:
      None

      Description

      The guava classes go as a dependency for jobs using TableMapReduceUtil from the org.apache.hadoop.hbase.mapred package but the same doesn't happen for jobs using the same class from org.apache.hadoop.hbase.mapreduce package. Due to this, tasks of the mapreduce jobs that wants to communicate with a secure cluster fails. The stack trace of the failing tasks:

      Error: java.lang.ClassNotFoundException: com.google.common.collect.ImmutableSet
      at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
      at java.security.AccessController.doPrivileged(Native Method)
      at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
      at java.lang.ClassLoader.loadClass(ClassLoader.java:306)
      at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
      at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
      at org.apache.hadoop.hbase.ipc.SecureServer.<clinit>(SecureServer.java:96)

      We just need to add the dependency on the guava classes in the mapreduce.TableMapReduceUtil class.

      1. 7851-1.patch
        0.7 kB
        Devaraj Das

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #12 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/12/)
        HBASE-7851. Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468)

        Result = FAILURE
        ddas :
        Files :

        • /hbase/branches/0.94/CHANGES.txt
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #12 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/12/ ) HBASE-7851 . Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468) Result = FAILURE ddas : Files : /hbase/branches/0.94/CHANGES.txt /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #111 (See https://builds.apache.org/job/HBase-0.94-security/111/)
        HBASE-7851. Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468)

        Result = SUCCESS
        ddas :
        Files :

        • /hbase/branches/0.94/CHANGES.txt
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #111 (See https://builds.apache.org/job/HBase-0.94-security/111/ ) HBASE-7851 . Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468) Result = SUCCESS ddas : Files : /hbase/branches/0.94/CHANGES.txt /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Hide
        Lars Hofhansl added a comment -

        Devaraj Das I see the actual patch committed also includes a change to CHANGES.txt. That is no longer necessary, as I generate the change log at the time of the release.

        Show
        Lars Hofhansl added a comment - Devaraj Das I see the actual patch committed also includes a change to CHANGES.txt. That is no longer necessary, as I generate the change log at the time of the release.
        Hide
        stack added a comment -

        OK. Thanks for entertaining my comment.

        Show
        stack added a comment - OK. Thanks for entertaining my comment.
        Hide
        Devaraj Das added a comment -

        This change doesn't impact trunk. In trunk, this situation doesn't arise. I think we should leave this issue as it is for 0.94. As Gary pointed out, there are other things to take care of if we remove guava dependency.

        Show
        Devaraj Das added a comment - This change doesn't impact trunk. In trunk, this situation doesn't arise. I think we should leave this issue as it is for 0.94. As Gary pointed out, there are other things to take care of if we remove guava dependency.
        Hide
        Lars Hofhansl added a comment -

        Hmm... It's easy enough to express the change in HBASE-5735 without using Guava.
        So in this specific case removing that seems to be better approach.

        Or we just do further changes in trunk and leave this be as is in 0.94.

        Show
        Lars Hofhansl added a comment - Hmm... It's easy enough to express the change in HBASE-5735 without using Guava. So in this specific case removing that seems to be better approach. Or we just do further changes in trunk and leave this be as is in 0.94.
        Hide
        Gary Helmling added a comment -

        The Guava dependency in SecureServer apparently came in with HBASE-5735. Looking around, there are quite a few other dependencies on Guava that might be exposed to clients, though, including in o.a.h.h.util.Bytes, and in KeyValue. Is it still worth extracting at this point?

        Show
        Gary Helmling added a comment - The Guava dependency in SecureServer apparently came in with HBASE-5735 . Looking around, there are quite a few other dependencies on Guava that might be exposed to clients, though, including in o.a.h.h.util.Bytes, and in KeyValue. Is it still worth extracting at this point?
        Hide
        stack added a comment -

        Here is the issue where we stripped back guava: HBASE-3264 Sounds like it was not comprehensive enough stripping (i.e. missed security). The notion that we minimize MR dependencies and that we try to strip guava because its not heavily used comes through in the issue.

        Show
        stack added a comment - Here is the issue where we stripped back guava: HBASE-3264 Sounds like it was not comprehensive enough stripping (i.e. missed security). The notion that we minimize MR dependencies and that we try to strip guava because its not heavily used comes through in the issue.
        Hide
        Devaraj Das added a comment -

        Just a note that the guava dependency was already there in the mapred.TableMapReduceUtil class. This patch just makes the mapreduce.TableMapReduceUtil be in sync with that. I guess if we decide to remove the guava dependency, we should do the same in both places..

        Show
        Devaraj Das added a comment - Just a note that the guava dependency was already there in the mapred.TableMapReduceUtil class. This patch just makes the mapreduce.TableMapReduceUtil be in sync with that. I guess if we decide to remove the guava dependency, we should do the same in both places..
        Hide
        Devaraj Das added a comment -

        stack, the guava dependency comes from a reference to it from SecureServer.java (look at the stack trace in the jira description). This problem shouldn't happen on trunk since there is no SecureServer and the RPC client code-path doesn't reference anything from guava there.

        Now coming back to the point on removing guava as a dependency, I wasn't aware of it. In theory, the patch could have been written to remove the reference to the guava class in SecureServer.java and instead implement the required functionality using JDK classes... What do you think?

        Show
        Devaraj Das added a comment - stack , the guava dependency comes from a reference to it from SecureServer.java (look at the stack trace in the jira description). This problem shouldn't happen on trunk since there is no SecureServer and the RPC client code-path doesn't reference anything from guava there. Now coming back to the point on removing guava as a dependency, I wasn't aware of it. In theory, the patch could have been written to remove the reference to the guava class in SecureServer.java and instead implement the required functionality using JDK classes... What do you think?
        Hide
        stack added a comment -

        Did we figure what needs guava? We try to minimize dependencies required by mr and in the past we worked to remove guava as a dependency after it was introduced as a dependency for what turned out to be a non-critical need. Perhaps we did not eliminate it everywhere or it snuck back in?

        Show
        stack added a comment - Did we figure what needs guava? We try to minimize dependencies required by mr and in the past we worked to remove guava as a dependency after it was introduced as a dependency for what turned out to be a non-critical need. Perhaps we did not eliminate it everywhere or it snuck back in?
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #843 (See https://builds.apache.org/job/HBase-0.94/843/)
        HBASE-7851. Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468)

        Result = SUCCESS
        ddas :
        Files :

        • /hbase/branches/0.94/CHANGES.txt
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #843 (See https://builds.apache.org/job/HBase-0.94/843/ ) HBASE-7851 . Include the guava classes as a dependency for jobs using mapreduce.TableMapReduceUtil (ddas). (Revision 1446468) Result = SUCCESS ddas : Files : /hbase/branches/0.94/CHANGES.txt /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/mapreduce/TableMapReduceUtil.java
        Hide
        Devaraj Das added a comment -

        Committed to 0.94.

        Show
        Devaraj Das added a comment - Committed to 0.94.
        Hide
        Lars Hofhansl added a comment -

        +1

        Show
        Lars Hofhansl added a comment - +1
        Hide
        Ted Yu added a comment -

        +1 from me.

        Show
        Ted Yu added a comment - +1 from me.
        Hide
        Sergey Shelukhin added a comment -

        +1

        Show
        Sergey Shelukhin added a comment - +1
        Hide
        Devaraj Das added a comment -

        The patch for 0.94. This issue doesn't affect trunk.

        Show
        Devaraj Das added a comment - The patch for 0.94. This issue doesn't affect trunk.

          People

          • Assignee:
            Devaraj Das
            Reporter:
            Devaraj Das
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development