[SPARK-31831][SQL][TESTS] Use subclasses for mock in HiveSessionImplSuite #29069

ukby1234 · 2020-07-10T19:17:28Z

What changes were proposed in this pull request?

Fix flaky test org.apache.spark.sql.hive.thriftserver.HiveSessionImplSuite by using subclasses to avoid classloader issue.

Why are the changes needed?

It causes build instability.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

It is a fix for a flaky test, but need to run multiple times against Jenkins.

ukby1234 · 2020-07-10T21:42:58Z

Still an issue in https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125621/testReport/org.apache.spark.sql.hive.thriftserver/HiveSessionImplSuite/_It_is_not_a_test_it_is_a_sbt_testing_SuiteSelector_/

dongjoon-hyun · 2020-07-10T22:34:58Z

cc @HeartSaVioR

HeartSaVioR · 2020-07-11T00:50:01Z

Unfortunately it turned out my patch didn't fix the issue (so I'm OK to revert my patch if we want to) - that said, I'd like to see the analysis how it fixes the issue if the approach still relies on mock. I'd be comfortable if the approach uses subclassing.

I'm seeing frequent failures on it - probably it's time to consider disabling the suite?

ukby1234 · 2020-07-11T00:59:17Z

I think the default one uses ObjenesisInstantiator which uses StdInstantiatorStrategy that goes pretty deep in vendor-specific JVM. I switched them to use ConstructorInstantiator which just uses plain java reflection to do object instantiation. If the mock .class generated has interface MockAccess, this is certainly the safest route to construct such instance.

ukby1234 · 2020-07-11T01:01:12Z

that's the code for StdInstantiatorStrategy:

public <T> ObjectInstantiator<T> newInstantiatorOf(Class<T> type) {

      if(PlatformDescription.isThisJVM(HOTSPOT) || PlatformDescription.isThisJVM(OPENJDK)) {
         // Java 7 GAE was under a security manager so we use a degraded system
         if(PlatformDescription.isGoogleAppEngine() && PlatformDescription.SPECIFICATION_VERSION.equals("1.7")) {
            if(Serializable.class.isAssignableFrom(type)) {
               return new ObjectInputStreamInstantiator<T>(type);
            }
            return new AccessibleInstantiator<T>(type);
         }
         // The UnsafeFactoryInstantiator would also work. But according to benchmarks, it is 2.5
         // times slower. So I prefer to use this one
         return new SunReflectionFactoryInstantiator<T>(type);
      }
      else if(PlatformDescription.isThisJVM(DALVIK)) {
         if(PlatformDescription.isAndroidOpenJDK()) {
            // Starting at Android N which is based on OpenJDK
            return new UnsafeFactoryInstantiator<T>(type);
         }
         if(ANDROID_VERSION <= 10) {
            // Android 2.3 Gingerbread and lower
            return new Android10Instantiator<T>(type);
         }
         if(ANDROID_VERSION <= 17) {
            // Android 3.0 Honeycomb to 4.2 Jelly Bean
            return new Android17Instantiator<T>(type);
         }
         // Android 4.3 until Android N
         return new Android18Instantiator<T>(type);
      }
      else if(PlatformDescription.isThisJVM(JROCKIT)) {
         // JRockit is compliant with HotSpot
         return new SunReflectionFactoryInstantiator<T>(type);
      }
      else if(PlatformDescription.isThisJVM(GNU)) {
         return new GCJInstantiator<T>(type);
      }
      else if(PlatformDescription.isThisJVM(PERC)) {
         return new PercInstantiator<T>(type);
      }

      // Fallback instantiator, should work with most modern JVM
      return new UnsafeFactoryInstantiator<T>(type);

   }

ukby1234 · 2020-07-11T01:03:40Z

Turns out our class of mock doesn't have a complicated constructor.

HeartSaVioR · 2020-07-11T08:53:39Z

ok to test

HeartSaVioR · 2020-07-11T09:49:19Z

I don't know about the details in mockito mock mechanism, but according to the error message and the stack trace, my bet for the problematic spot is SubclassBytecodeGenerator.mockClass, and this patch also does go though the path.

I'd say we may need to probably check the thread context classloader just before calling mock, and whether it can find MockAccess. That's why I'd like to just avoid mocking and see manual subclass to just make test work.

HyukjinKwon · 2020-07-11T15:22:17Z

ok to test

SparkQA · 2020-07-11T16:03:31Z

Test build #125686 has finished for PR 29069 at commit c120f0c.

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

ukby1234 · 2020-07-12T00:01:46Z

Yeah, I changed to use the subclasses for mocking. I think it is a classloader issue with threading.

SparkQA · 2020-07-12T00:03:34Z

Test build #125697 has finished for PR 29069 at commit 7c668fe.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class GetCatalogsOperationMock extends GetCatalogsOperation
public class OperationManagerMock extends OperationManager

SparkQA · 2020-07-12T00:19:46Z

Test build #125698 has finished for PR 29069 at commit 9067f21.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class GetCatalogsOperationMock extends GetCatalogsOperation
public class OperationManagerMock extends OperationManager

SparkQA · 2020-07-12T01:39:12Z

Test build #125701 has finished for PR 29069 at commit 6771504.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class GetCatalogsOperationMock extends GetCatalogsOperation
public class OperationManagerMock extends OperationManager

SparkQA · 2020-07-12T02:47:31Z

Test build #125702 has finished for PR 29069 at commit 611ba1b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class GetCatalogsOperationMock extends GetCatalogsOperation
public class OperationManagerMock extends OperationManager

HeartSaVioR · 2020-07-12T08:24:48Z

@frankyin-factual
Thanks for the update. The approach looks good. Would you mind if we just put everything in the suite, so that we don't need to find things which is only used at once?

Below change (+ imports) would make you get rid of new java files:

class GetCatalogsOperationMock(parentSession: HiveSession)
  extends GetCatalogsOperation(parentSession) {

  override def runInternal(): Unit = {}

  override def getHandle: OperationHandle = {
    val uuid: UUID = UUID.randomUUID();
    val tHandleIdentifier: THandleIdentifier = new THandleIdentifier()
    tHandleIdentifier.setGuid(getByteBufferFromUUID(uuid));
    tHandleIdentifier.setSecret(getByteBufferFromUUID(uuid));
    val tOperationHandle: TOperationHandle = new TOperationHandle()
    tOperationHandle.setOperationId(tHandleIdentifier);
    tOperationHandle.setOperationType(TOperationType.GET_TYPE_INFO);
    tOperationHandle.setHasResultSetIsSet(false);
    new OperationHandle(tOperationHandle);
  }

  private def getByteBufferFromUUID(uuid: UUID): Array[Byte] = {
    val bb: ByteBuffer = ByteBuffer.wrap(new Array[Byte](16))
    bb.putLong(uuid.getMostSignificantBits())
    bb.putLong(uuid.getLeastSignificantBits())
    bb.array();
  }
}

class OperationManagerMock extends OperationManager {
  private val calledHandles: mutable.Set[OperationHandle] = new mutable.HashSet[OperationHandle]()

  override def newGetCatalogsOperation(parentSession: HiveSession): GetCatalogsOperation = {
    val operation = new GetCatalogsOperationMock(parentSession)
    try {
      val m = classOf[OperationManager].getDeclaredMethod("addOperation", classOf[Operation])
      m.setAccessible(true)
      m.invoke(this, operation)
    } catch {
      case e@(_: NoSuchMethodException | _: IllegalAccessException |
              _: InvocationTargetException) =>
        throw new RuntimeException(e)
    }
    operation
  }

  override def closeOperation(opHandle: OperationHandle): Unit = {
    calledHandles.add(opHandle)
    throw new RuntimeException
  }

  def getCalledHandles: mutable.Set[OperationHandle] = calledHandles
}

ukby1234 · 2020-07-12T08:55:29Z

Just pushed such changes.

HeartSaVioR

LGTM pending Jenkins

SparkQA · 2020-07-12T09:34:21Z

Test build #125713 has finished for PR 29069 at commit 1eb2027.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class GetCatalogsOperationMock(parentSession: HiveSession)
class OperationManagerMock extends OperationManager

dongjoon-hyun · 2020-07-12T15:50:28Z

Thanks for working on this, @HeartSaVioR and @frankyin-factual .

HeartSaVioR · 2020-07-12T20:06:59Z

Thanks! Merged into master.

dongjoon-hyun · 2020-07-16T02:08:29Z

Hi, @HeartSaVioR and @frankyin-factual .
This seems to break master compilation on Hive 1.2 profile. Could you take a look at the failure?

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-hive-1.2/631/

[ERROR] [Error] /home/jenkins/workspace/spark-master-test-maven-hadoop-2.7-hive-1.2/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala:30: object rpc is not a member of package org.apache.hive.service
[ERROR] [Error] /home/jenkins/workspace/spark-master-test-maven-hadoop-2.7-hive-1.2/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala:75: not found: type THandleIdentifier
[ERROR] [Error] /home/jenkins/workspace/spark-master-test-maven-hadoop-2.7-hive-1.2/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala:75: not found: type THandleIdentifier
[ERROR] [Error] /home/jenkins/workspace/spark-master-test-maven-hadoop-2.7-hive-1.2/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala:78: not found: type TOperationHandle
[ERROR] [Error] /home/jenkins/workspace/spark-master-test-maven-hadoop-2.7-hive-1.2/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala:78: not found: type TOperationHandle

dongjoon-hyun · 2020-07-16T02:09:16Z

...hriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveSessionImplSuite.scala

-import org.mockito.invocation.InvocationOnMock
+import org.apache.hive.service.cli.operation.{GetCatalogsOperation, Operation, OperationManager}
+import org.apache.hive.service.cli.session.{HiveSession, HiveSessionImpl, SessionManager}
+import org.apache.hive.service.rpc.thrift.{THandleIdentifier, TOperationHandle, TOperationType}


According to the compilation error, this import is invalid in Hive 1.2.

dongjoon-hyun · 2020-07-16T02:11:38Z

cc @gatorsmile since he has been interested in Hive 1.2 profile.

HyukjinKwon · 2020-07-16T03:15:53Z

Let's revert if it's non trivial to fix @HeartSaVioR.

HeartSaVioR · 2020-07-16T03:36:50Z

I'll take a look at it today. We can revert it if there's no good way to fix.

ukby1234 · 2020-07-16T03:44:19Z

I will also take a look.

HeartSaVioR · 2020-07-16T04:01:57Z

I guess we have several possible approaches here:

place the suite to the Hive-version specific directory (with new config on pom.xml to add the test source based on version)
create shim and place the shim to the Hive-version specific directory (same)
revert and try to place the suite on the list of running in separate JVM (less chance to encounter classloader issue)

I guess the straightforward approach would be 1 - I guess it should work smoothly, though if we want to ensure the suite runs in both versions we'll end up with duplicating codes. If that doesn't matter much we can just do that, or even we can just enable the test on hive 2.3 only (as the suite is technically irrelevant to Hive 1.2 vs Hive 2.3).

Less redundant but more complicated would be 2. I'm not 100% sure how much complexity is needed to make a shim for the suite, and not sure it worths to do instead of simply allowing redundant codes a bit.

The simplest approach would be 3, but it's not guaranteed to fix the flakiness. I'd say it'd be better to leave as the last resort.

WDYT?

dongjoon-hyun · 2020-07-16T04:31:41Z

Thank you. I'm fine for all combination (including Hive 2.3 only testing). Please feel free to choose an option. From my side, this also looks not urgent since this is not blocking both GitHub Action and PRBuilder. It has been broken over 3 days already. I hope Hive 1.2 is going to be removed in the near future eventually after we build a consensus. Sooner is better.

In short, please proceed toward what you think is right, @HeartSaVioR .

ukby1234 · 2020-07-16T04:43:39Z

I am working on a combination of 1) and 2). Will push shortly.

ukby1234 · 2020-07-16T04:53:23Z

@HeartSaVioR @dongjoon-hyun #29129

…e in hive version related subdirectories ### What changes were proposed in this pull request? This patch fixes the build issue on Hive 1.2 profile brought by #29069, via putting mocks for HiveSessionImplSuite in hive version related subdirectories, so that maven build will pick up the proper source code according to the profile. ### Why are the changes needed? #29069 fixed the flakiness of HiveSessionImplSuite, but given the patch relied on the default profile (Hive 2.3) it broke the build with Hive 1.2 profile. This patch addresses both Hive versions. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Manually confirmed the test suite via below command: > Hive 1.2 ``` build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.sql.hive.thriftserver.HiveSessionImplSuite test -Phive-1.2 -Phadoop-2.7 -Phive-thriftserver ``` > Hive 2.3 ``` build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.sql.hive.thriftserver.HiveSessionImplSuite test -Phive-2.3 -Phadoop-3.2 -Phive-thriftserver ``` Closes #29129 from frankyin-factual/hive-tests. Authored-by: Frank Yin <[email protected]> Signed-off-by: Jungtaek Lim (HeartSaVioR) <[email protected]>

ukby1234 changed the title ~~[SPARK-31831] Use constructor for mock in HiveSessionImplSuite~~ [SPARK-31831][SQL][TESTS] Use constructor for mock in HiveSessionImplSuite Jul 10, 2020

ukby1234 force-pushed the hive-tests branch from c120f0c to 7c668fe Compare July 12, 2020 00:01

probot-autolabeler bot added the SQL label Jul 12, 2020

ukby1234 force-pushed the hive-tests branch from 7c668fe to 9067f21 Compare July 12, 2020 00:07

ukby1234 changed the title ~~[SPARK-31831][SQL][TESTS] Use constructor for mock in HiveSessionImplSuite~~ [SPARK-31831][SQL][TESTS] Use subclasses for mock in HiveSessionImplSuite Jul 12, 2020

ukby1234 force-pushed the hive-tests branch from 9067f21 to 6771504 Compare July 12, 2020 01:25

ukby1234 force-pushed the hive-tests branch from 6771504 to 611ba1b Compare July 12, 2020 02:09

[SPARK-31831] Use subclasses for mock in HiveSessionImplSuite

1eb2027

ukby1234 force-pushed the hive-tests branch from 611ba1b to 1eb2027 Compare July 12, 2020 08:55

HeartSaVioR approved these changes Jul 12, 2020

View reviewed changes

dongjoon-hyun added the TESTS label Jul 12, 2020

HeartSaVioR closed this in ad90cbf Jul 12, 2020

dongjoon-hyun reviewed Jul 16, 2020

View reviewed changes

ukby1234 mentioned this pull request Jul 16, 2020

[SPARK-31831][SQL][TESTS][FOLLOWUP] Put mocks for HiveSessionImplSuite in hive version related subdirectories #29129

Closed

[SPARK-31831][SQL][TESTS] Use subclasses for mock in HiveSessionImplSuite #29069

[SPARK-31831][SQL][TESTS] Use subclasses for mock in HiveSessionImplSuite #29069

Uh oh!

Conversation

ukby1234 commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

ukby1234 commented Jul 10, 2020

Uh oh!

dongjoon-hyun commented Jul 10, 2020

Uh oh!

HeartSaVioR commented Jul 11, 2020

Uh oh!

ukby1234 commented Jul 11, 2020

Uh oh!

ukby1234 commented Jul 11, 2020

Uh oh!

ukby1234 commented Jul 11, 2020

Uh oh!

HeartSaVioR commented Jul 11, 2020

Uh oh!

HeartSaVioR commented Jul 11, 2020

Uh oh!

HyukjinKwon commented Jul 11, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

ukby1234 commented Jul 12, 2020

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

HeartSaVioR commented Jul 12, 2020

Uh oh!

ukby1234 commented Jul 12, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 12, 2020

Uh oh!

dongjoon-hyun commented Jul 12, 2020

Uh oh!

HeartSaVioR commented Jul 12, 2020

Uh oh!

dongjoon-hyun commented Jul 16, 2020

Uh oh!

dongjoon-hyun Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 16, 2020

Uh oh!

HyukjinKwon commented Jul 16, 2020

Uh oh!

HeartSaVioR commented Jul 16, 2020

Uh oh!

ukby1234 commented Jul 16, 2020

Uh oh!

HeartSaVioR commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 16, 2020

Uh oh!

ukby1234 commented Jul 16, 2020

Uh oh!

ukby1234 commented Jul 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

ukby1234 commented Jul 10, 2020 •

edited

Loading

HeartSaVioR commented Jul 16, 2020 •

edited

Loading