Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-13648] Add Hive Cli to classes for isolated classloader #11495

Closed
wants to merge 1 commit into from

Conversation

preecet
Copy link
Contributor

@preecet preecet commented Mar 3, 2016

What changes were proposed in this pull request?

Adding the hive-cli classes to the classloader

How was this patch tested?

The hive Versionssuite tests were run

This is my original work and I license the work to the project under the project's open source license.

@preecet
Copy link
Contributor Author

preecet commented Mar 3, 2016

Copy of recent development mailing list comment.

I have been testing 1.6.1RC1 using the IBM Java SDK.

I notice a problem ( with the org.apache.spark.sql.hive.client.VersionsSuite tests ) after a recent Spark 1.6.1 change.
Pull request - f7898f9

The change introduced a dependency on org.apache.hadoop.hive.cli.CliSessionState in org.apache.spark.sql.hive.client.ClientWrapper.

In particular the following test was added
if (originalState.isInstanceOf[CliSessionState]) {

The problem is that VersionsSuite test uses an isolated classloader in order to test various versions of Hive. However the classpath of the isolated classloader does not contain CliSessionState.

The behaviour of isInstanceOf[CliSessionState]) is JVM vendor specific. In particular whether this code causes the CliSessionState class to be loaded. ( It does not for openjdk, but does for IBM JDK ). Hence this call can throw a classnotfound exception.

@rxin
Copy link
Contributor

rxin commented Mar 3, 2016

cc @marmbrus

@marmbrus
Copy link
Contributor

marmbrus commented Mar 3, 2016

ok to test

@marmbrus
Copy link
Contributor

marmbrus commented Mar 3, 2016

LGTM pending tests

@SparkQA
Copy link

SparkQA commented Mar 4, 2016

Test build #52418 has finished for PR 11495 at commit 79911ff.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

marmbrus commented Mar 7, 2016

Thanks, merging to master and 1.6.

@asfgit asfgit closed this in 46f25c2 Mar 7, 2016
asfgit pushed a commit that referenced this pull request Mar 7, 2016
## What changes were proposed in this pull request?

Adding the hive-cli classes to the classloader

## How was this patch tested?

The hive Versionssuite tests were run

This is my original work and I license the work to the project under the project's open source license.

Author: Tim Preece <[email protected]>

Closes #11495 from preecet/master.

(cherry picked from commit 46f25c2)
Signed-off-by: Michael Armbrust <[email protected]>
@adrian-wang
Copy link
Contributor

Why we need hive-cli here? This is not version specific.

@preecet
Copy link
Contributor Author

preecet commented Mar 8, 2016

Because ClientWrapper calls isInstanceOf[CliSessionState] and so needs to load the class CliSessionstate. In the testcase ClientWraper has been loaded by the isolated class loader.

roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
## What changes were proposed in this pull request?

Adding the hive-cli classes to the classloader

## How was this patch tested?

The hive Versionssuite tests were run

This is my original work and I license the work to the project under the project's open source license.

Author: Tim Preece <[email protected]>

Closes apache#11495 from preecet/master.
asfgit pushed a commit that referenced this pull request Apr 21, 2016
The `hive` subproject currently depends on `hive-cli` in order to perform a check to see whether a `SessionState` is an instance of `org.apache.hadoop.hive.cli.CliSessionState` (see #9589). The introduction of this `hive-cli` dependency has caused problems for users whose Hive metastore JAR classpaths don't include the `hive-cli` classes (such as in #11495).

This patch removes this dependency on `hive-cli` and replaces the `isInstanceOf` check by reflection. I added a Maven Enforcer rule to ban `hive-cli` from the `hive` subproject in order to make sure that this dependency is not accidentally reintroduced.

/cc rxin yhuai adrian-wang preecet

Author: Josh Rosen <[email protected]>

Closes #12551 from JoshRosen/remove-hive-cli-dep-from-hive-subproject.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants