[SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2][test-maven] Fix concurrency issue for ThriftCLIService to getPortNumber #28835

yaooqinn · 2020-06-15T16:21:11Z

What changes were proposed in this pull request?

This PR brings #28751 back

It once reverted by 4a25200 because of inevitable maven test failure
- See related updates in this followup a0187cd
And reverted again because of the flakiness of the added unit tests
- In this PR, The flakiness reason found is caused by the hive metastore connection that the SparkSQLCLIService trying to create which turns out is unnecessary at all. This metastore client points to a dummy metastore server only.
- Also, add some cleanups for SharedThriftServer trait in before and after to prevent its configurations being polluted or polluting others

Why are the changes needed?

fix flaky test

Does this PR introduce any user-facing change?

no

How was this patch tested?

passing sbt and maven tests

dongjoon-hyun · 2020-06-15T16:23:26Z

Thank you for investigating this, @yaooqinn .
cc @cloud-fan

yaooqinn · 2020-06-15T17:07:50Z

retest this please

SparkQA · 2020-06-15T17:09:42Z

Test build #124064 has finished for PR 28835 at commit f9ea941.

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

SparkQA · 2020-06-15T17:29:42Z

Test build #124067 has finished for PR 28835 at commit 03fcf59.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-15T17:40:34Z

Test build #124068 has finished for PR 28835 at commit 7908d62.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-15T23:55:33Z

Test build #124069 has finished for PR 28835 at commit 7908d62.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-06-16T02:02:16Z

Sorry, @yaooqinn and @cloud-fan . It seems that we had better revert the patch first because it's too flaky.

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/

yaooqinn · 2020-06-16T02:21:27Z

Thanks, @dongjoon-hyun, and sorry for the inconvenience. I will keep working on this.

…urrency issue for ThriftCLIService to getPortNumber"" This reverts commit 75afd88.

yaooqinn · 2020-06-16T03:49:58Z

retest this please

SparkQA · 2020-06-16T06:27:42Z

Test build #124088 has finished for PR 28835 at commit 9c6ac83.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-06-16T06:40:30Z

retest this please

SparkQA · 2020-06-16T07:05:01Z

Test build #124087 has finished for PR 28835 at commit 9c6ac83.

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

SparkQA · 2020-06-16T07:05:01Z

Test build #124102 has finished for PR 28835 at commit 9c6ac83.

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

yaooqinn · 2020-06-16T07:10:43Z

retest this please

SparkQA · 2020-06-16T08:42:43Z

Test build #124113 has started for PR 28835 at commit 5e79ad9.

yaooqinn · 2020-06-16T10:17:24Z

...-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIService.scala

+   * the super class [[CLIService#start]] starts a useless dummy metastore client, skip it and call
+   * the ancestor [[CompositeService#start]] directly.
+   */
+  override def start(): Unit = startCompositeService()


CLIService will create a metastore connection during start, which is useless for our dummy execution hive conf and will cause class cast issue though different classloader

Here we bypass it and start the registered services as CompositeService does

SparkQA · 2020-06-16T10:17:48Z

Test build #124119 has started for PR 28835 at commit 9cdd7fa.

SparkQA · 2020-06-16T15:10:17Z

Test build #124108 has finished for PR 28835 at commit 9c6ac83.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-06-16T17:52:44Z

retest this please

yaooqinn · 2020-06-17T06:31:30Z

retest this please

yaooqinn · 2020-06-17T06:35:43Z

retest this please

SparkQA · 2020-06-17T06:36:38Z

Test build #124157 has finished for PR 28835 at commit 5ce343a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-17T06:43:46Z

Test build #124158 has finished for PR 28835 at commit 5ce343a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-17T07:05:01Z

Test build #124150 has finished for PR 28835 at commit 5ce343a.

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

yaooqinn · 2020-06-17T09:17:03Z

retest this please

SparkQA · 2020-06-17T09:23:04Z

Test build #124162 has finished for PR 28835 at commit 5ce343a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2020-06-18T04:51:47Z

cc @juliuszsompolski

yaooqinn · 2020-06-18T07:06:53Z

retest this please

juliuszsompolski

Thanks for digging into this!

juliuszsompolski · 2020-06-18T09:50:27Z

...-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIService.scala

+      throw new ServiceException("Failed to Start " + getName, e)
    }
+
+    // Emulating `AbstractService.start`


AbstractService.start does also startTime = System.currentTimeMillis();
Lets set startTime as well, just in case.

Nice catch!

juliuszsompolski · 2020-06-18T09:52:08Z

...-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SharedThriftServer.scala

+    sqlContext.setConf(ConfVars.METASTORECONNECTURLKEY.varname,
+      s"jdbc:derby:;databaseName=$metastorePath;create=true")
+    sqlContext.setConf(ConfVars.METASTOREURIS.varname, "")


What's this for?

Some test failures showed that the metastore client was trying to connect through METASTOREURIS which seems to be set by other tests. But now I guess this won't be necessary as we skip that part

SparkQA · 2020-06-18T09:56:39Z

Test build #124202 has finished for PR 28835 at commit 5ce343a.

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

SparkQA · 2020-06-18T13:44:56Z

Test build #124215 has finished for PR 28835 at commit d5341ea.

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

juliuszsompolski · 2020-06-18T14:03:07Z

...-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIService.scala

+    // Emulating `AbstractService.start`
+    val startTime = new java.lang.Long(System.currentTimeMillis())
+    setAncestorField(this, 3, "startTime", startTime)
+    invoke(classOf[AbstractService], this, "ensureCurrentState", classOf[STATE] -> STATE.INITED)


This can throw IllegalStateException, and in the original implementation it is inside the try catch block, which would turn it into ServiceException.
Let's do that as well just in case :-).

juliuszsompolski

LGTM

SparkQA · 2020-06-18T14:38:01Z

Test build #124218 has finished for PR 28835 at commit e409f9a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-06-18T17:26:51Z

retest this please

SparkQA · 2020-06-18T20:30:46Z

Test build #124228 has finished for PR 28835 at commit e409f9a.

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

yaooqinn · 2020-06-19T02:30:06Z

retest this please

yaooqinn · 2020-06-19T02:32:06Z

retest this please

SparkQA · 2020-06-19T05:41:47Z

Test build #124249 has finished for PR 28835 at commit e409f9a.

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

cloud-fan · 2020-06-19T05:58:47Z

thanks, merging to master! (don't backport as it's not a test only PR anymore)

SparkQA · 2020-06-19T07:05:02Z

Test build #124250 has finished for PR 28835 at commit e409f9a.

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

probot-autolabeler bot added the SQL label Jun 15, 2020

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP] Cleanup the thread local variable of hive metastore~~ [WIP][SPARK-31926][TESTS][FOLLOWUP][test-maven] Cleanup the thread local variable of hive metastore Jun 15, 2020

dongjoon-hyun mentioned this pull request Jun 16, 2020

[SPARK-31926][SQL][TEST-HIVE1.2][test-maven] Fix concurrency issue for ThriftCLIService to getPortNumber #28797

Closed

yaooqinn added 2 commits June 16, 2020 11:40

Revert "Revert "[SPARK-31926][SQL][TEST-HIVE1.2][TEST-MAVEN] Fix conc…

03f41ab

…urrency issue for ThriftCLIService to getPortNumber"" This reverts commit 75afd88.

rebase and classloader update

9c6ac83

probot-autolabeler bot added the BUILD label Jun 16, 2020

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP][test-maven] Cleanup the thread local variable of hive metastore~~ [WIP][SPARK-31926][TESTS][FOLLOWUP] Cleanup the thread local variable of hive metastore Jun 16, 2020

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP] Cleanup the thread local variable of hive metastore~~ [WIP][SPARK-31926][TESTS][FOLLOWUP][test-maven] Cleanup the thread local variable of hive metastore Jun 16, 2020

override start method

5e79ad9

won't reach class loder part anymore

9cdd7fa

yaooqinn commented Jun 16, 2020

View reviewed changes

throw

5ce343a

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP][test-maven] Cleanup the thread local variable of hive metastore~~ [WIP][SPARK-31926][TESTS][FOLLOWUP]Cleanup the thread local variable of hive metastore Jun 17, 2020

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP]Cleanup the thread local variable of hive metastore~~ [WIP][SPARK-31926][TESTS][FOLLOWUP] Fix concurrency issue for ThriftCLIService to getPortNumber Jun 18, 2020

juliuszsompolski reviewed Jun 18, 2020

View reviewed changes

start time

d5341ea

juliuszsompolski reviewed Jun 18, 2020

View reviewed changes

yaooqinn changed the title ~~[WIP][SPARK-31926][TESTS][FOLLOWUP] Fix concurrency issue for ThriftCLIService to getPortNumber~~ [SPARK-31926][SQL][TESTS][FOLLOWUP] Fix concurrency issue for ThriftCLIService to getPortNumber Jun 18, 2020

move service.start into try-catch

e409f9a

juliuszsompolski approved these changes Jun 18, 2020

View reviewed changes

yaooqinn changed the title ~~[SPARK-31926][SQL][TESTS][FOLLOWUP] Fix concurrency issue for ThriftCLIService to getPortNumber~~ [SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2] Fix concurrency issue for ThriftCLIService to getPortNumber Jun 19, 2020

yaooqinn changed the title ~~[SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2] Fix concurrency issue for ThriftCLIService to getPortNumber~~ [SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2][test-maven] Fix concurrency issue for ThriftCLIService to getPortNumber Jun 19, 2020

cloud-fan closed this in abc8ccc Jun 19, 2020

[SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2][test-maven] Fix concurrency issue for ThriftCLIService to getPortNumber #28835

[SPARK-31926][SQL][TESTS][FOLLOWUP][test-hive1.2][test-maven] Fix concurrency issue for ThriftCLIService to getPortNumber #28835

Uh oh!

Conversation

yaooqinn commented Jun 15, 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!

dongjoon-hyun commented Jun 15, 2020

Uh oh!

yaooqinn commented Jun 15, 2020

Uh oh!

SparkQA commented Jun 15, 2020

Uh oh!

SparkQA commented Jun 15, 2020

Uh oh!

SparkQA commented Jun 15, 2020

Uh oh!

SparkQA commented Jun 15, 2020

Uh oh!

dongjoon-hyun commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 16, 2020

Uh oh!

yaooqinn commented Jun 17, 2020

Uh oh!

yaooqinn commented Jun 17, 2020

Uh oh!

SparkQA commented Jun 17, 2020

Uh oh!

SparkQA commented Jun 17, 2020

Uh oh!

SparkQA commented Jun 17, 2020

Uh oh!

yaooqinn commented Jun 17, 2020

Uh oh!

SparkQA commented Jun 17, 2020

Uh oh!

gatorsmile commented Jun 18, 2020

Uh oh!

yaooqinn commented Jun 18, 2020

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Jun 15, 2020 •

edited

Loading