[SPARK-23831][SQL] Add org.apache.derby to IsolatedClientLoader #20944

wangyum · 2018-03-30T10:50:22Z

What changes were proposed in this pull request?

Add org.apache.derby to IsolatedClientLoader, otherwise it may throw an exception:

...
[info] Cause: java.sql.SQLException: Failed to start database 'metastore_db' with class loader org.apache.spark.sql.hive.client.IsolatedClientLoader$$anon$1@2439ab23, see the next exception for details.
[info] at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
[info] at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
[info] at org.apache.derby.impl.jdbc.Util.seeNextException(Unknown Source)
[info] at org.apache.derby.impl.jdbc.EmbedConnection.bootDatabase(Unknown Source)
[info] at org.apache.derby.impl.jdbc.EmbedConnection.<init>(Unknown Source)
[info] at org.apache.derby.jdbc.InternalDriver$1.run(Unknown Source)
...

How was this patch tested?

unit tests and manual tests

SparkQA · 2018-03-30T13:14:50Z

Test build #88748 has finished for PR 20944 at commit 7d5cc71.

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

felixcheung · 2018-04-03T06:02:01Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

@@ -188,6 +188,9 @@ private[hive] class IsolatedClientLoader(
    (name.startsWith("com.google") && !name.startsWith("com.google.cloud")) ||
    name.startsWith("java.lang.") ||
    name.startsWith("java.net") ||
+    name.startsWith("com.sun.") ||
+    name.startsWith("sun.reflect.") ||


these two lines above are really broad, is this intentional?

Yes, it doesn't matter if add these two lines, but I think it's best to add. What do you think?

Do not add them unless we have to do it.

wangyum · 2018-04-08T01:45:44Z

cc @jerryshao

gatorsmile · 2018-04-08T05:34:59Z

@wangyum What is the root cause?

felixcheung · 2018-04-08T06:50:33Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

@@ -188,6 +188,9 @@ private[hive] class IsolatedClientLoader(
    (name.startsWith("com.google") && !name.startsWith("com.google.cloud")) ||
    name.startsWith("java.lang.") ||
    name.startsWith("java.net") ||
+    name.startsWith("com.sun.") ||
+    name.startsWith("sun.reflect.") ||
+    name.startsWith("org.apache.derby.") ||


I'd also move this line to other org.apache* above

jerryshao · 2018-04-08T06:54:21Z

Please list out the reason why do you need such change? If it is a UT bug, why it didn't happen before?

SparkQA · 2018-04-09T07:05:01Z

Test build #89043 has finished for PR 20944 at commit 1c801f1.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2018-04-09T07:08:20Z

retest this please.

SparkQA · 2018-04-09T09:45:44Z

Test build #89046 has finished for PR 20944 at commit 1c801f1.

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

SparkQA · 2018-06-21T16:16:57Z

Test build #92170 has finished for PR 20944 at commit da36564.

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

SparkQA · 2018-06-21T16:31:36Z

Test build #92171 has finished for PR 20944 at commit 7133d7a.

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

holdenk · 2018-06-27T21:39:46Z

So I had this come up while I was testing Spark 2.1.3 RC2 on a machine with an existing YARN cluster with spark-testing-base. Haven't had the chance to dig into it fully.

holdenk · 2018-07-13T17:50:35Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

@@ -182,6 +182,7 @@ private[hive] class IsolatedClientLoader(
    name.startsWith("org.slf4j") ||
    name.startsWith("org.apache.log4j") || // log4j1.x
    name.startsWith("org.apache.logging.log4j") || // log4j2
+    name.startsWith("org.apache.derby.") ||


So to clarify, this adds derby to the "shared" classes (we might want to mention this above on L139) and presumably this fix is for cases where we have an existing derby version on the class-path which may differ and if that happens we end up with strangeness on the metastore? cc @gatorsmile ?

Like I said, I've run into this issue so I'd like to see a fix :)

gatorsmile · 2018-07-13T21:07:01Z

LGTM

Thanks! Merged to master

HyukjinKwon · 2018-11-07T07:31:43Z

Sorry, why was this change required? I don't see #20944 (comment) is addressed Can you elaborate please? Why do we make org.apache.derby as shared?

Ideally, minor or maintenance versions of derby can be dumped up, and they shouldn't be shared unless there's a strong reason to keep it shared, for instance, fixing class resolution failed. How did you reproduce this and why the unit test is not added?

I found an actual issue while working on Apache Livy Spark 2.4 support. I am still investigating how it relates with the test failures but at the very least I see this specific commit matters since Apache Livy unittests pass without this specific commit.

Adding @vanzin and @mgaido91

HyukjinKwon · 2018-11-07T07:45:05Z

Please describe manual tests and how it relates to actual usecase.

HyukjinKwon · 2018-11-07T22:21:03Z

ping @wangyum, I'm going to revert this today if there's no response today.

wangyum · 2018-11-08T02:17:00Z

Sorry @HyukjinKwon It's difficult reproduce. I am not sure whether it is caused by multithreading.
But you can verify it by:

spark/sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogSuite.scala

Lines 117 to 120 in a75571b

    
           test("SPARK-23831: Add org.apache.derby to IsolatedClientLoader") { 
        
             val client1 = HiveUtils.newClientForMetadata(new SparkConf, new Configuration) 
        
             val client2 = HiveUtils.newClientForMetadata(new SparkConf, new Configuration) 
        
             assert(!client1.equals(client2))

HyukjinKwon · 2018-11-08T03:57:19Z

I understood the reproducer step in JIRA but how and why it matters? Did it cause an actual problem in your production environment?

HyukjinKwon · 2018-11-08T03:59:19Z

To me, it sounds we made a fix but it was difficult to figure out exactly what's going on internally. It's okay if it's difficult to reproduce but it can be reproduced in production; however, the problem is that the fix of this caused another problem.

I am asking those to see why and how important this fix was.

wangyum · 2018-11-08T08:17:47Z

This fix for testing only, production won't use derby as their matestore database.

HyukjinKwon · 2018-11-08T08:30:48Z

Let's revert this then if this only targeted to fix the test. We can bring this back later when it's needed - tho, yea . This caused a specific case failure in Livy' when restarting Hive enabled Spark session.

@gatorsmile, I will revert this but I don't mind getting this back again if it actually fixes a usecase since I either don't know how exactly this fixes the case above and how it causes to make the Livy case failed - one thing clear is this specific commit is the cause.

Please revert me action if I missed an actual usecase fixed by this, I am okay.

wangyum · 2019-04-08T03:17:53Z

@HyukjinKwon I could reproduce this issue:

build/sbt clean package -Phive -Phive-thriftserver
export SPARK_PREPEND_CLASSES=true
bin/spark-sql --conf spark.sql.hive.metastore.version=2.3.4 --conf spark.sql.hive.metastore.jars=maven -e "create table t1 as select 1 as c"

HyukjinKwon · 2019-04-08T04:12:31Z

So this isn't test only fix anymore? If so, let's get it back. Let me check it too soon.

HyukjinKwon · 2019-04-11T00:57:38Z

Hm, but I still think #20944 (comment) is valid, actually. @wangyum, do we meet this issue when we use our binary releases, or did you meet this only in dev as described above?

wangyum · 2019-04-11T02:54:31Z

I hint this issue in two places:

Specify spark.sql.hive.metastore.version when using derby:

wget https://www-eu.apache.org/dist/spark/spark-2.4.1/spark-2.4.1-bin-hadoop2.7.tgz && tar -zxf spark-2.4.1-bin-hadoop2.7.tgz && cd spark-2.4.1-bin-hadoop2.7
bin/spark-sql --conf spark.sql.hive.metastore.version=2.0.0 --conf spark.sql.hive.metastore.jars=maven -e "create table t1 as select 1 as c"

Unit testing when the built-in Hive is upgraded to 2.3([WIP][test-hadoop3.2] Test Hadoop 3.2 on jenkins #24044. Remove this line and run):

build/sbt  "hive/testOnly *.HiveExternalSessionCatalogSuite *.HiveExternalCatalogSuite *.MultiDatabaseSuite"  -Phive -Phadoop-3.2

HyukjinKwon · 2019-04-11T04:38:58Z

I see. But can you check if Hive 3.1 support and onward still works with this change? My impression is that we will don't respect their derby version with this change anymore.

wangyum · 2019-04-15T10:50:59Z

The root cause is: Hive creates more java.sql.Connection since HIVE-10632(Hive 2.1.0).

Add org.apache.derby to IsolatedClientLoader

7d5cc71

felixcheung reviewed Apr 3, 2018

View reviewed changes

felixcheung reviewed Apr 8, 2018

View reviewed changes

Remove com.sun. and sun.reflect.

1c801f1

wangyum added 2 commits June 21, 2018 21:39

Merge remote-tracking branch 'upstream/master' into SPARK-23831

da36564

Add test case

7133d7a

holdenk reviewed Jul 13, 2018

View reviewed changes

asfgit closed this in a75571b Jul 13, 2018

gatorsmile mentioned this pull request Jan 15, 2019

[DO-NOT-MERGE][SPARK-23710] Upgrade built-in Hive to 2.3.4(Without hive-thriftserver) #23552

Closed

HyukjinKwon mentioned this pull request Apr 18, 2019

[SPARK-27402][SQL][test-hadoop3.2][test-maven] Fix hadoop-3.2 test issue(except the hive-thriftserver module) #24391

Closed

HyukjinKwon mentioned this pull request Mar 22, 2020

[SPARK-31203][BUILD] Upgrade derby to 10.14.2.0 from 10.12.1.1 #27970

Closed

wangyum deleted the SPARK-23831 branch September 4, 2020 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23831][SQL] Add org.apache.derby to IsolatedClientLoader #20944

[SPARK-23831][SQL] Add org.apache.derby to IsolatedClientLoader #20944

wangyum commented Mar 30, 2018 •

edited

Loading

SparkQA commented Mar 30, 2018

felixcheung Apr 3, 2018

wangyum Apr 4, 2018

gatorsmile Apr 8, 2018

wangyum commented Apr 8, 2018

gatorsmile commented Apr 8, 2018

felixcheung Apr 8, 2018

jerryshao commented Apr 8, 2018

SparkQA commented Apr 9, 2018

wangyum commented Apr 9, 2018

SparkQA commented Apr 9, 2018

SparkQA commented Jun 21, 2018

SparkQA commented Jun 21, 2018

holdenk commented Jun 27, 2018

holdenk Jul 13, 2018

gatorsmile commented Jul 13, 2018

HyukjinKwon commented Nov 7, 2018 •

edited

Loading

HyukjinKwon commented Nov 7, 2018

HyukjinKwon commented Nov 7, 2018

wangyum commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018 •

edited

Loading

wangyum commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018

wangyum commented Apr 8, 2019

HyukjinKwon commented Apr 8, 2019

HyukjinKwon commented Apr 11, 2019

wangyum commented Apr 11, 2019

HyukjinKwon commented Apr 11, 2019

wangyum commented Apr 15, 2019

[SPARK-23831][SQL] Add org.apache.derby to IsolatedClientLoader #20944

[SPARK-23831][SQL] Add org.apache.derby to IsolatedClientLoader #20944

Conversation

wangyum commented Mar 30, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Mar 30, 2018

felixcheung Apr 3, 2018

Choose a reason for hiding this comment

wangyum Apr 4, 2018

Choose a reason for hiding this comment

gatorsmile Apr 8, 2018

Choose a reason for hiding this comment

wangyum commented Apr 8, 2018

gatorsmile commented Apr 8, 2018

felixcheung Apr 8, 2018

Choose a reason for hiding this comment

jerryshao commented Apr 8, 2018

SparkQA commented Apr 9, 2018

wangyum commented Apr 9, 2018

SparkQA commented Apr 9, 2018

SparkQA commented Jun 21, 2018

SparkQA commented Jun 21, 2018

holdenk commented Jun 27, 2018

holdenk Jul 13, 2018

Choose a reason for hiding this comment

gatorsmile commented Jul 13, 2018

HyukjinKwon commented Nov 7, 2018 • edited Loading

HyukjinKwon commented Nov 7, 2018

HyukjinKwon commented Nov 7, 2018

wangyum commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018 • edited Loading

wangyum commented Nov 8, 2018

HyukjinKwon commented Nov 8, 2018

wangyum commented Apr 8, 2019

HyukjinKwon commented Apr 8, 2019

HyukjinKwon commented Apr 11, 2019

wangyum commented Apr 11, 2019

HyukjinKwon commented Apr 11, 2019

wangyum commented Apr 15, 2019

wangyum commented Mar 30, 2018 •

edited

Loading

HyukjinKwon commented Nov 7, 2018 •

edited

Loading

HyukjinKwon commented Nov 8, 2018 •

edited

Loading