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-32909][SQL] Pass all sql/hive-thriftserver module UTs in Scala 2.13 #29783

Closed

Conversation

LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Sep 17, 2020

What changes were proposed in this pull request?

This pr fix failed and aborted cases in sql hive-thriftserver module in Scala 2.13, the main change of this pr as follow:

  • Use s.c.Seq instead of Seq in HiveResult because the input type maybe mutable.ArraySeq, but Seq represent immutable.Seq in Scala 2.13.

  • Reset classLoader after HiveMetastoreLazyInitializationSuite completed because context class loader is NonClosableMutableURLClassLoader in HiveMetastoreLazyInitializationSuite running process, and it propagate to HiveThriftServer2ListenerSuite trigger following problems in Scala 2.13:

HiveThriftServer2ListenerSuite:
*** RUN ABORTED ***
  java.lang.LinkageError: loader constraint violation: loader (instance of net/bytebuddy/dynamic/loading/MultipleParentClassLoader) previously initiated loading for a different type with name "org/apache/hive/service/ServiceStateChangeListener"
  at org.mockito.codegen.HiveThriftServer2$MockitoMock$1850222569.<clinit>(Unknown Source)
  at sun.reflect.GeneratedSerializationConstructorAccessor530.newInstance(Unknown Source)
  at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
  at org.objenesis.instantiator.sun.SunReflectionFactoryInstantiator.newInstance(SunReflectionFactoryInstantiator.java:48)
  at org.objenesis.ObjenesisBase.newInstance(ObjenesisBase.java:73)
  at org.mockito.internal.creation.instance.ObjenesisInstantiator.newInstance(ObjenesisInstantiator.java:19)
  at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMock(SubclassByteBuddyMockMaker.java:47)
  at org.mockito.internal.creation.bytebuddy.ByteBuddyMockMaker.createMock(ByteBuddyMockMaker.java:25)
  at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:35)
  at org.mockito.internal.MockitoCore.mock(MockitoCore.java:63)
  ...

After this pr HiveThriftServer2Suites and HiveThriftServer2ListenerSuite was fixed and all 461 test passed

Why are the changes needed?

We need to support a Scala 2.13 build.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Scala 2.12: Pass the Jenkins or GitHub Action

  • Scala 2.13: All tests passed.

Do the following:

dev/change-scala-version.sh 2.13
mvn clean install -DskipTests -pl sql/hive-thriftserver -am -Phive-thriftserver -Pscala-2.13
mvn test -pl sql/hive-thriftserver -Phive -Phive-thriftserver -Pscala-2.13

Before

HiveThriftServer2ListenerSuite:
*** RUN ABORTED ***

After

Tests: succeeded 461, failed 0, canceled 0, ignored 17, pending 0
All tests passed.

@@ -31,6 +31,7 @@ class HiveMetastoreLazyInitializationSuite extends SparkFunSuite {
.config("spark.hadoop.hive.metastore.uris", "thrift://127.0.0.1:11111")
.getOrCreate()
val originalLevel = org.apache.log4j.Logger.getRootLogger().getLevel
val originalClassLoader = Thread.currentThread().getContextClassLoader
Copy link
Contributor Author

@LuciferYang LuciferYang Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @srowen I think this change is very Interesting, HiveThriftServer2ListenerSuite will ABORTED without this change

HiveThriftServer2ListenerSuite:
*** RUN ABORTED ***
  java.lang.LinkageError: loader constraint violation: loader (instance of net/bytebuddy/dynamic/loading/MultipleParentClassLoader) previously initiated loading for a different type with name "org/apache/hive/service/ServiceStateChangeListener"

The Context Class Loader is NonClosableMutableURLClassLoader in HiveMetastoreLazyInitializationSuite running process and propagate to HiveThriftServer2ListenerSuite , then

if (live) {
val server = mock(classOf[HiveThriftServer2], RETURNS_SMART_NULLS)
val listener = new HiveThriftServer2Listener(kvstore, sparkConf, Some(server))
(new HiveThriftServer2AppStatusStore(kvstore, Some(listener)), listener)

will use MultipleParentClassLoader(NonClosableMutableURLClassLoader and AppClassLoader) to mock HiveThriftServer2 instance in Scala 2.13, but mock HiveThriftServer2 instance use AppClassLoader only in Scala 2.12.

Then I do some experiments:

  • Ignore HiveMetastoreLazyInitializationSuite all tests passed

  • Run HiveThriftServer2ListenerSuite only will passed

  • Copy test case HiveMetastoreLazyInitializationSuite to HiveThriftServer2ListenerSuite then run HiveThriftServer2ListenerSuite only, it aborted in both Scala 2.12 and Scala 2.13.

I guess that HiveMetastoreLazyInitializationSuite and HiveThriftServer2ListenerSuite run in same thread in Scala 2.13 but different thread in Scala 2.12, but I don't know why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for investigation, @LuciferYang . It's interesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I think it's fine, as this change just improves isolation in any event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya. I agree with @srowen . The patch itself looks good.

@SparkQA
Copy link

SparkQA commented Sep 17, 2020

Test build #128811 has finished for PR 29783 at commit 0d20878.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-32909][SQL] Pass all sql/hive-thriftserver module UTs in Scala 2.13 [SPARK-32909][SQL][test-java11] Pass all sql/hive-thriftserver module UTs in Scala 2.13 Sep 17, 2020
@dongjoon-hyun
Copy link
Member

Retest this please

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I triggered JDK11 testing too because this is a class loader related issue, @LuciferYang .

@SparkQA
Copy link

SparkQA commented Sep 17, 2020

Test build #128831 has finished for PR 29783 at commit 0d20878.

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

@dongjoon-hyun
Copy link
Member

Thank you, @LuciferYang and @srowen .
Merged to master for Apache Spark 3.1.0 on December 2020.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-32909][SQL][test-java11] Pass all sql/hive-thriftserver module UTs in Scala 2.13 [SPARK-32909][SQL] Pass all sql/hive-thriftserver module UTs in Scala 2.13 Sep 17, 2020
@LuciferYang
Copy link
Contributor Author

Thank @srowen and @dongjoon-hyun ~

@LuciferYang LuciferYang deleted the sql-thriftserver-tests branch June 6, 2022 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants