You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by gaborgsomogyi <gi...@git.apache.org> on 2017/12/05 11:28:50 UTC
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
GitHub user gaborgsomogyi opened a pull request:
https://github.com/apache/spark/pull/19893
[SPARK-16139][TEST] Add logging functionality for leaked threads in tests
## What changes were proposed in this pull request?
Lots of our tests don't properly shutdown everything they create, and end up leaking lots of threads. For example, `TaskSetManagerSuite` doesn't stop the extra `TaskScheduler` and `DAGScheduler` it creates. There are a couple more instances, eg. in `DAGSchedulerSuite`.
This PR adds the possibility to print out the not properly stopped thread list after a test suite executed. The format is the following:
```
===== FINISHED o.a.s.scheduler.DAGSchedulerSuite: 'task end event should have updated accumulators (SPARK-20342)' =====
...
===== Global thread whitelist loaded with name /thread_whitelist from classpath: rpc-client.*, rpc-server.*, shuffle-client.*, shuffle-server.*' =====
ScalaTest-run:
===== THREADS NOT STOPPED PROPERLY =====
ScalaTest-run: dag-scheduler-event-loop
ScalaTest-run: globalEventExecutor-2-5
ScalaTest-run:
===== END OF THREAD DUMP =====
ScalaTest-run:
===== EITHER PUT THREAD NAME INTO THE WHITELIST FILE OR SHUT IT DOWN PROPERLY =====
```
With the help of this leaking threads has been identified in TaskSetManagerSuite. My intention is to hunt down and fix such bugs in later PRs.
## How was this patch tested?
Manual: TaskSetManagerSuite test executed and found out where are the leaking threads.
Automated: Pass the Jenkins.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gaborgsomogyi/spark SPARK-16139
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19893.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19893
----
commit 9acbb62f90656321f6db3a52e5c69fd1745dba7f
Author: Gabor Somogyi <ga...@gmail.com>
Date: 2017-12-05T08:52:55Z
[SPARK-16139][TEST] Add logging functionality for leaked threads in tests
commit 9058603a815fac49af969ff595b43223b6f34f73
Author: Gabor Somogyi <ga...@gmail.com>
Date: 2017-12-05T09:16:07Z
Apache license header added
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155592201
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
Yes, but why is it ok to whitelist them?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r160158241
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -27,19 +27,55 @@ import org.apache.spark.util.AccumulatorContext
/**
* Base abstract class for all unit tests in Spark for handling common functionality.
+ *
+ * Thread audit happens normally here automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
--- End diff --
Changed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84858 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84858/testReport)** for PR 19893 at commit [`ef58576`](https://github.com/apache/spark/commit/ef58576d953ac98e29027477e9638e4508ee5ced).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159838691
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/sources/DataSourceAnalysisSuite.scala ---
@@ -29,16 +29,24 @@ import org.apache.spark.sql.types.{DataType, IntegerType, StructType}
class DataSourceAnalysisSuite extends SparkFunSuite with BeforeAndAfterAll {
+ protected override val doThreadAuditInSparkFunSuite = false
+
private var targetAttributes: Seq[Attribute] = _
private var targetPartitionSchema: StructType = _
override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/19893
lgtm
@jiangxb1987 are you still looking at this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85315/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86035/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158513693
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
+ val shortSuiteName = suiteName.replaceAll("org.apache.spark", "o.a.s")
+
+ if (threadNamesSnapshot.nonEmpty) {
+ val remainingThreadNames = runningThreadNames.diff(threadNamesSnapshot)
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ if (remainingThreadNames.nonEmpty) {
+ logWarning(s"\n\n===== POSSIBLE THREAD LEAK IN SUITE $shortSuiteName, " +
+ s"thread names: ${remainingThreadNames.mkString(", ")} =====\n")
+ }
+ } else {
+ logWarning(s"\n\n===== THREAD AUDIT POST ACTION CALLED " +
--- End diff --
nit: remove 's' before the string.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155386336
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,24 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ "rpc-client.*", "rpc-server.*", "shuffle-client.*", "shuffle-server.*",
--- End diff --
Temporarily removed rpc and shuffle. I'll put them back when proper doc can be written.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r160158067
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,22 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Auto thread audit is turned off here intentionally and done manually.
--- End diff --
Same thing but it meant to solve different problem (changes the execution order). Please see the execution order with and without this change described in my previous post:
```
As a next step analysed SQL test flow. Here are the steps:
1. SharedSparkSession.beforeAll called which initialise SparkSession and SQLContext
2. SparkFunSuite.beforeAll creates a thread snapshot
3. Test code runs
4. SparkFunSuite.afterAll prints out the possible leaks
5. SharedSparkSession.afterAll stops SparkSession
Not sure if I understand right but this will not report false positives. The only problem what I see here as it's not gonna report SparkSession and SQLContext related leaks.
As you mentioned before this code should find SparkContext related threading issues which applies here as well. This is not fulfilled at the moment and my proposal is to fix it this way:
1. SparkFunSuite.beforeAll creates a thread snapshot
2. SharedSparkSession.beforeAll called which initialise SparkSession and SQLContext
3. Test code runs
4. SharedSparkSession.afterAll stops SparkSession
5. SparkFunSuite.afterAll prints out the possible leaks
With this change I don't see any false positives and missed threads.
Please share your ideas related this topic.
```
Your concern is fully standing but this change is not intended to cover the mentioned issue. The problem you mentioned is addressed in ThreadAudit, namely it prints out the following message such cases:
```
THREAD AUDIT POST ACTION CALLED WITHOUT PRE ACTION IN SUITE...
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158514388
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,17 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ override protected val doThreadAuditInSparkFunSuite = false
+
+ protected override def beforeAll(): Unit = {
+ doThreadPreAudit
+ super.beforeAll
--- End diff --
nit: `super.beforeAll()`, and also `super.afterAll()`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155556989
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,24 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ "rpc-client.*", "rpc-server.*", "shuffle-client.*", "shuffle-server.*",
--- End diff --
I've made deepdive what these threads are and put documentation for each. I'll execute a build with them and let's see the new numbers.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155366384
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -72,3 +99,27 @@ abstract class SparkFunSuite
}
}
+
+object SparkFunSuite
+ extends Logging {
--- End diff --
move to previous line
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159567782
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -31,15 +31,28 @@ import org.apache.spark.util.AccumulatorContext
abstract class SparkFunSuite
extends FunSuite
with BeforeAndAfterAll
+ with ThreadAudit
with Logging {
// scalastyle:on
+ protected val doThreadAuditInSparkFunSuite = true
--- End diff --
Given the way this is being used elsewhere, a better name is probably `enableAutoThreadAudit`. or something.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r160501177
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,22 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Auto thread audit is turned off here intentionally and done manually.
--- End diff --
I'm not sure I understand your explanation, and I definitely don't understand what's going on from the comment in the code. What I'm asking is for the comment here to explain not what the code is doing, but *why* it's doing it.
Basically, if instead of the code you have here, you just called `super.beforeAll` and `super.afterAll`, without disabling `enableAutoThreadAudit`, what will break and why? That's what the comment should explain.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84858 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84858/testReport)** for PR 19893 at commit [`ef58576`](https://github.com/apache/spark/commit/ef58576d953ac98e29027477e9638e4508ee5ced).
* This patch **fails to generate documentation**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84617 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84617/testReport)** for PR 19893 at commit [`644ee6a`](https://github.com/apache/spark/commit/644ee6aec14faa89789dd9ef52fb539e93a7076d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86094 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86094/testReport)** for PR 19893 at commit [`68d0f3b`](https://github.com/apache/spark/commit/68d0f3beac63fefba436646896d5cd82a3c4da21).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84577 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84577/testReport)** for PR 19893 at commit [`a35a52f`](https://github.com/apache/spark/commit/a35a52f6da3301c8675cec85bebc95d474d25718).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@vanzin I've fixed the SQL test flow and additionally I've made the implementation less invasive by extracting the logic into a trait.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84869/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159851476
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,17 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ override protected val doThreadAuditInSparkFunSuite = false
+
+ protected override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
It's kind of similar but not the same. Comment added.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86035 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86035/testReport)** for PR 19893 at commit [`9c9c6ef`](https://github.com/apache/spark/commit/9c9c6ef05e6ed6d1ad1a50591a0ed84811876bd7).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84580 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84580/testReport)** for PR 19893 at commit [`fe6cd0c`](https://github.com/apache/spark/commit/fe6cd0cc727a9d4516f6af674e616f0b1f8cef93).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155591920
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
--- End diff --
But why are these whitelisted?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159831366
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
--- End diff --
Good point, moving.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
I've just started to take a look at it deeper and found some patterns. Namely we can exclude all netty.* threads + ForkJoinPool.* is most of the time but not always created inside scala by the global ExecutionContext. All in all far from have a good picture but I'll exclude these entries.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156385068
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
In the meantime analyzed the related threads and here are the findings:
/**
* During [[SparkContext]] creation [[org.apache.spark.storage.BlockManager]]
* creates event loops. One is wrapped inside
* [[org.apache.spark.network.server.TransportServer]]
* the other one is inside [[org.apache.spark.network.client.TransportClient]].
* The thread pools behind shut down asynchronously triggered by [[SparkContext#close]].
* Manually checked and all of them stopped properly.
*/
"shuffle-client.*",
"shuffle-server.*"
Do you think in this situation they can be added to the whitelist?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159523352
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -31,15 +31,28 @@ import org.apache.spark.util.AccumulatorContext
abstract class SparkFunSuite
extends FunSuite
with BeforeAndAfterAll
+ with ThreadAudit
with Logging {
// scalastyle:on
+ protected val doThreadAuditInSparkFunSuite = true
--- End diff --
Can we call this just `doThreadAudit` or `enableThreadAudit`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155598719
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
--- End diff --
This is netty internal what we're not configuring or using directly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84869 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84869/testReport)** for PR 19893 at commit [`827b6dc`](https://github.com/apache/spark/commit/827b6dca818d3f678c74f0edda04a1f9b315f932).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159527791
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,17 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ override protected val doThreadAuditInSparkFunSuite = false
+
+ protected override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
This looks like the same situation, but because this is a trait, it kinda relies on the suites to call `beforeAll` and `afterAll` correctly... if you don't want to audit all suites, you could write a comment explaining the situation.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
> Those try to keep the same session alive for multiple suites
Good point to make this part clear.
As a first step I've taken a look at the code and as I see SparkSession as well as SQLContext recreated between different suites. They're shared inside a suite. Have I missed something?
```
Class: org.apache.spark.sql.test.GenericFlatSpecSuite SparkSession: org.apache.spark.sql.test.TestSparkSession@1b5bc39d SQLContext: org.apache.spark.sql.SQLContext@655a5d9c
Class: org.apache.spark.sql.test.GenericWordSpecSuite SparkSession: org.apache.spark.sql.test.TestSparkSession@53cddaf8 SQLContext: org.apache.spark.sql.SQLContext@55c50f52
Class: org.apache.spark.sql.test.GenericFunSpecSuite SparkSession: org.apache.spark.sql.test.TestSparkSession@22ff1372 SQLContext: org.apache.spark.sql.SQLContext@356341c9
Class: org.apache.spark.sql.test.DataFrameReaderWriterSuite SparkSession: org.apache.spark.sql.test.TestSparkSession@2c02a007 SQLContext: org.apache.spark.sql.SQLContext@63a72cc6
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159954825
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -27,19 +27,55 @@ import org.apache.spark.util.AccumulatorContext
/**
* Base abstract class for all unit tests in Spark for handling common functionality.
+ *
+ * Thread audit happens normally here automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
--- End diff --
Better:
"
It is possible to override the default thread audit behavior by setting `enableAutoThreadAudit` to false and manually calling the audit methods, if desired. For example:
// Code
"
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159524812
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
--- End diff --
Same reasoning as above. Just inline.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159523662
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
--- End diff --
You shouldn't describe the behavior of `SparkFunSuite` here. You should instead document the flag in `SparkFunSuite` that controls whether this code is triggered.
All the comments in the rest of this class are related to `SparkFunSuite` and overriding its default behavior, so they're better placed in `SparkFunSuite` and not here too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
Merging to master.
It would be nice to file a separate bug to eventually look at how to do this on the spark-hive module (or maybe it's just not worth the effort).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85290 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85290/testReport)** for PR 19893 at commit [`ef00796`](https://github.com/apache/spark/commit/ef007969a576dd5f84440ce78a325d6617dc5278).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r160158308
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -27,19 +27,55 @@ import org.apache.spark.util.AccumulatorContext
/**
* Base abstract class for all unit tests in Spark for handling common functionality.
+ *
+ * Thread audit happens normally here automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
--- End diff --
Changed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155370650
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
--- End diff --
Moved.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159841944
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -31,15 +31,28 @@ import org.apache.spark.util.AccumulatorContext
abstract class SparkFunSuite
extends FunSuite
with BeforeAndAfterAll
+ with ThreadAudit
with Logging {
// scalastyle:on
+ protected val doThreadAuditInSparkFunSuite = true
--- End diff --
I was thinking about proper naming before. The last suggested one is definitely better. No exact place where it happens but not suggesting that it's completely turned off.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85722 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85722/testReport)** for PR 19893 at commit [`0851ef2`](https://github.com/apache/spark/commit/0851ef2b572ed507155358e541a4fa53a8e92173).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158527212
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
+ val shortSuiteName = suiteName.replaceAll("org.apache.spark", "o.a.s")
+
+ if (threadNamesSnapshot.nonEmpty) {
+ val remainingThreadNames = runningThreadNames.diff(threadNamesSnapshot)
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ if (remainingThreadNames.nonEmpty) {
+ logWarning(s"\n\n===== POSSIBLE THREAD LEAK IN SUITE $shortSuiteName, " +
+ s"thread names: ${remainingThreadNames.mkString(", ")} =====\n")
+ }
+ } else {
+ logWarning(s"\n\n===== THREAD AUDIT POST ACTION CALLED " +
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86094/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155600929
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
These threads started when SparkContext created and remain there. These can be destroyed when sc.stop() called but I've seen this pattern rarely. If we would like to follow this, huge amount of tests should be modified and I don't see the ROI. On the other hand not whitelisting them would trigger false positives when context tried to be reused like you mentioned in case of hive. Ideas/opinions? Shall we drop them?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84763 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84763/testReport)** for PR 19893 at commit [`3bde33c`](https://github.com/apache/spark/commit/3bde33c481a35257497a1b9d4787bdc9ab7c7a3c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159834709
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
--- End diff --
Inlined.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159524447
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
--- End diff --
nit: line wrapped too early.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84915 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84915/testReport)** for PR 19893 at commit [`ef00796`](https://github.com/apache/spark/commit/ef007969a576dd5f84440ce78a325d6617dc5278).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r154957048
--- Diff: core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala ---
@@ -683,7 +683,7 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg
val conf = new SparkConf().set("spark.speculation", "true")
sc = new SparkContext("local", "test", conf)
- val sched = new FakeTaskScheduler(sc, ("execA", "host1"), ("execB", "host2"))
+ sched = new FakeTaskScheduler(sc, ("execA", "host1"), ("execB", "host2"))
--- End diff --
What was this change about? not shadowing?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161293095
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
Sorry but this still does not explain why this is happening. It's just stating that it is.
For example, in `SharedSparkSession`, there is this code:
```
protected override def afterAll(): Unit = {
super.afterAll()
if (_spark != null) {
_spark.sessionState.catalog.reset()
_spark.stop()
_spark = null
}
}
```
If you move `super.afterAll()` to after the session is stopped, won't that solve the problem and avoid this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
> With this change I don't see any false positives and missed threads.
That sounds good but it only covers sql. Did you also look at hive as I mentioned previously?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159840754
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSessionStateSuite.scala ---
@@ -28,14 +28,18 @@ import org.apache.spark.sql.hive.test.TestHiveSingleton
class HiveSessionStateSuite extends SessionStateSuite
with TestHiveSingleton with BeforeAndAfterEach {
+ override protected val doThreadAuditInSparkFunSuite = false
+
override def beforeAll(): Unit = {
// Reuse the singleton session
activeSession = spark
+ doThreadPreAudit()
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/19893
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155605695
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
Fully agree with your stand and tests should stop their context. What I see is that majority of tests are just not doing that. OK, then I suggest to remove them from the whitelist. Agreed?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/19893
ok I just took a look at BroadcastExchangeExec, I see what you mean. It isn't *that* bad, since spark isn't continually creating more instances of those threads (you're not supposed to have more than one spark context in one jvm anyway ...). But it also means a bad test could mess up that thread pool (send it some task which never completes) and it would screw up all future tests. I'd file a jira about it, but don't think you need to worry about it for this change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155596370
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
--- End diff --
One example is netty-rpc... related internal threads where we have no control what's going on in the background.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
As a next step analysed SQL test flow. Here are the steps:
1. SharedSparkSession.beforeAll called which initialise SparkSession and SQLContext
2. SparkFunSuite.beforeAll creates a thread snapshot
3. Test code runs
4. SparkFunSuite.afterAll prints out the possible leaks
5. SharedSparkSession.afterAll stops SparkSession
Not sure if I understand right but this will not report false positives. The only problem what I see here as it's not gonna report SparkSession and SQLContext related leaks.
As you mentioned before this code should find SparkContext related threading issues which applies here as well. This is not fulfilled at the moment and my proposal is to fix it this way:
1. SparkFunSuite.beforeAll creates a thread snapshot
2. SharedSparkSession.beforeAll called which initialise SparkSession and SQLContext
3. Test code runs
4. SharedSparkSession.afterAll stops SparkSession
5. SparkFunSuite.afterAll prints out the possible leaks
With this change I don't see any false positives and missed threads.
Please share your ideas related this topic.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84907 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84907/testReport)** for PR 19893 at commit [`e2da334`](https://github.com/apache/spark/commit/e2da334560f764af71b6677f2e4c7c772063c229).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19893
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159954589
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -27,19 +27,55 @@ import org.apache.spark.util.AccumulatorContext
/**
* Base abstract class for all unit tests in Spark for handling common functionality.
+ *
+ * Thread audit happens normally here automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
--- End diff --
`enableAutoThreadAudit` now
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84606/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
Why not disable the thread audit in the hive module? You added that functionality already, should be pretty trivial to use it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84879/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@squito I mean another jira, because it needs deeper analysis and discussion.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159832729
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155373535
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,24 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ "rpc-client.*", "rpc-server.*", "shuffle-client.*", "shuffle-server.*",
--- End diff --
It would be nice to add comments explaining why the threads are whitelisted. Without an explanation to the contrary, I don't think any of these should be whitelisted.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84577/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155639667
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
--- End diff --
Explanation added.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84915/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86050/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155639821
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
Removed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159831631
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85802/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161222015
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,22 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Auto thread audit is turned off here intentionally and done manually.
--- End diff --
Yeah, now I see your point.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84577 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84577/testReport)** for PR 19893 at commit [`a35a52f`](https://github.com/apache/spark/commit/a35a52f6da3301c8675cec85bebc95d474d25718).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158527190
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84907 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84907/testReport)** for PR 19893 at commit [`e2da334`](https://github.com/apache/spark/commit/e2da334560f764af71b6677f2e4c7c772063c229).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Seems like the new feature caught some false positives in SQL:
```
===== THREAD AUDIT POST ACTION CALLED WITHOUT PRE ACTION IN SUITE o.a.s.sql.sources.DataSourceAnalysisSuite =====
===== THREAD AUDIT POST ACTION CALLED WITHOUT PRE ACTION IN SUITE o.a.s.sql.SessionStateSuite =====
```
Fixed them as well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159527868
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSessionStateSuite.scala ---
@@ -28,14 +28,18 @@ import org.apache.spark.sql.hive.test.TestHiveSingleton
class HiveSessionStateSuite extends SessionStateSuite
with TestHiveSingleton with BeforeAndAfterEach {
+ override protected val doThreadAuditInSparkFunSuite = false
+
override def beforeAll(): Unit = {
// Reuse the singleton session
activeSession = spark
+ doThreadPreAudit()
--- End diff --
Same thing. Just call `super.beforeAll()` correctly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84907/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155592072
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
--- End diff --
But are these stoppable or just out of our control? That's the kind of thing that should define whether something is in the whitelist or not.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86094 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86094/testReport)** for PR 19893 at commit [`68d0f3b`](https://github.com/apache/spark/commit/68d0f3beac63fefba436646896d5cd82a3c4da21).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Checked the failure but seems like unrelated.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85722 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85722/testReport)** for PR 19893 at commit [`0851ef2`](https://github.com/apache/spark/commit/0851ef2b572ed507155358e541a4fa53a8e92173).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159524570
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
--- End diff --
nit: call with `()` if you declare the method with `()`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@jiangxb1987 I don't know whether I understand your concern well but there is no intention to modify the shared `TestHiveContext ` among suites. It will remain as it is now. As an additional note, the plus functionality coming from this PR is only a printout which can be ignored any time.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84763/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155370597
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
--- End diff --
Config file removed and refactored as you suggested. It's much more simple now, thanks :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155603848
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
--- End diff --
Ah, OK :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/19893
Oh, it's great that the shared context don't cause much false positives. My only concern now is that the number of warnings is scary, could you please give me one or two days to look into the (possible) leaking thread issues to make sure there is no other obvious false positives?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155599671
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
--- End diff --
Note I want these explanations in the code comments, not here in the PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r154958833
--- Diff: core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala ---
@@ -683,7 +683,7 @@ class TaskSetManagerSuite extends SparkFunSuite with LocalSparkContext with Logg
val conf = new SparkConf().set("spark.speculation", "true")
sc = new SparkContext("local", "test", conf)
- val sched = new FakeTaskScheduler(sc, ("execA", "host1"), ("execB", "host2"))
+ sched = new FakeTaskScheduler(sc, ("execA", "host1"), ("execB", "host2"))
--- End diff --
Here originally the newly created instance was stored in a local variable which was never saved in member and freed properly. With this change the afterEach method stops it and frees up the resources.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86093 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86093/testReport)** for PR 19893 at commit [`56a41df`](https://github.com/apache/spark/commit/56a41dfece2513cd31ca10e7f4529d4b524a0814).
* This patch **fails Spark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds the following public classes _(experimental)_:
* ` * That trait initializes the spark session in its [[beforeAll()]] implementation before the`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155385446
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +66,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def runningThreadNames(): Set[String] = {
+ Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = runningThreadNames()
+ val whitelistedThreadNames = currentThreadNames
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ val remainingThreadNames = whitelistedThreadNames.diff(beforeAllTestThreadNames)
--- End diff --
Compressed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159527507
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/sources/DataSourceAnalysisSuite.scala ---
@@ -29,16 +29,24 @@ import org.apache.spark.sql.types.{DataType, IntegerType, StructType}
class DataSourceAnalysisSuite extends SparkFunSuite with BeforeAndAfterAll {
+ protected override val doThreadAuditInSparkFunSuite = false
+
private var targetAttributes: Seq[Attribute] = _
private var targetPartitionSchema: StructType = _
override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
Same thing here. This should be calling `super.beforeAll()`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
I have gathered statistics manually about the actual stand. I've grep-ed unit-test.logs in the whole build:
```
bash-3.2$ find . -type f | xargs grep "POSSIBLE THREAD LEAK" | wc -l
370
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85290 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85290/testReport)** for PR 19893 at commit [`ef00796`](https://github.com/apache/spark/commit/ef007969a576dd5f84440ce78a325d6617dc5278).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84601 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84601/testReport)** for PR 19893 at commit [`2b02d45`](https://github.com/apache/spark/commit/2b02d45303c018cae8e578804d07cf1803447c42).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155626333
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
Ahhh, that's a key. I've no doubt these whitelist items should be removed. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161322305
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
I think this is an easier to understand comment about what's going on here:
```
/**
* Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
* Trat trait initializes the spark session in its [[beforeAll()]] implementation before the
* automatic thread snapshot is performed, so the audit code could fail to report threads leaked
* by that shared session.
*
* The behavior is overridden here to take the snapshot before the spark session is initialized.
*/
```
Sorry for the noise.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #4006 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4006/testReport)** for PR 19893 at commit [`0d45a5b`](https://github.com/apache/spark/commit/0d45a5b7a225e33eedf13ee1f379ea64d6f53c18).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Yeah, this is fully true. This enhancement is definitely will not solve the issues once and for all. The problems were hidden till now and we would like to make a step ahead and make it at least visible somehow. With the help of this I've tracked down problems in additional 4-5 suites. All in all if you have an approach which solves context reuse feel free to share.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155608281
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
TaskManagerSuite.test("TaskSet with no preferences") prints this out:
===== POSSIBLE THREAD LEAK IN SUITE o.a.s.scheduler.TaskSetManagerSuite, thread names: rpc-client-1-1, rpc-server-3-2, rpc-client-1-4, rpc-server-3-6, rpc-client-1-3, rpc-server-3-7, rpc-server-3-1, rpc-server-3-8, rpc-server-3-4, rpc-client-1-7, rpc-client-1-6, rpc-client-1-2, rpc-server-3-5, shuffle-client-4-1, shuffle-server-5-1, rpc-server-3-3, rpc-client-1-5, rpc-client-1-8 =====
This is the result even if the test contains only this line:
sc = new SparkContext("local", "test")
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159842007
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -31,15 +31,28 @@ import org.apache.spark.util.AccumulatorContext
abstract class SparkFunSuite
extends FunSuite
with BeforeAndAfterAll
+ with ThreadAudit
with Logging {
// scalastyle:on
+ protected val doThreadAuditInSparkFunSuite = true
--- End diff --
Renamed to enableAutoThreadAudit.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155373350
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -20,10 +20,12 @@ package org.apache.spark
// scalastyle:off
import java.io.File
-import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
-
import org.apache.spark.internal.Logging
import org.apache.spark.util.AccumulatorContext
+import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
+
+import scala.collection.JavaConversions._
--- End diff --
Wonder why the style checker didn't complain, but `scala.*` imports should be in the previous position.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/19893
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156803714
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+// scalastyle:off
+import scala.collection.JavaConversions._
+
+import org.apache.spark.internal.Logging
+// scalastyle:on
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
+ val shortSuiteName = suiteName.replaceAll("org.apache.spark", "o.a.s")
+
+ if (threadNamesSnapshot.nonEmpty) {
+ val remainingThreadNames = runningThreadNames.diff(threadNamesSnapshot)
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ if (remainingThreadNames.nonEmpty) {
+ logWarning(s"\n\n===== POSSIBLE THREAD LEAK IN SUITE $shortSuiteName, " +
+ s"thread names: ${remainingThreadNames.mkString(", ")} =====\n")
+ }
+ } else {
+ logWarning(s"\n\n===== THREAD AUDIT POST ACTION CALLED " +
+ s"WITHOUT PRE ACTION IN SUITE $shortSuiteName =====\n")
+ }
+ }
+
+ private def runningThreadNames(): Set[String] = {
+ Thread.getAllStackTraces.keySet().map(_.getName).toSet
--- End diff --
and then here change to `keySet().asScala.map`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155639701
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
--- End diff --
Explanation added.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156385819
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
+ * For more details, see [[org.apache.spark.rpc.RpcEnv]].
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+ "dispatcher-event-loop.*",
+
+ /**
+ * These threads are created by spark when the BlockManager's BlockTransferService initialized
--- End diff --
In the meantime analyzed the related threads and here are the findings:
/**
* During [[SparkContext]] creation [[org.apache.spark.storage.BlockManager]]
* creates event loops. One is wrapped inside
* [[org.apache.spark.network.server.TransportServer]]
* the other one is inside [[org.apache.spark.network.client.TransportClient]].
* The thread pools behind shut down asynchronously triggered by [[SparkContext#close]].
* Manually checked and all of them stopped properly.
*/
"shuffle-client.*",
"shuffle-server.*"
Do you think in this situation they can be added to the whitelist?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84601 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84601/testReport)** for PR 19893 at commit [`2b02d45`](https://github.com/apache/spark/commit/2b02d45303c018cae8e578804d07cf1803447c42).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155370741
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
+ filterNot(s => SparkFunSuite.threadWhiteList.exists(s.matches(_)))
+ val remainingThreadNames = whitelistedThreadNames.diff(beforeAllTestThreadNames)
+ if (!remainingThreadNames.isEmpty) {
--- End diff --
Changed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85802 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85802/testReport)** for PR 19893 at commit [`87c4852`](https://github.com/apache/spark/commit/87c48520ef7c3236c217cdfd787e048866fcb75e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161308026
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
Your suggestion solves one part of the problem. The other one lies here:
```
protected override def beforeAll(): Unit = {
initializeSession()
// Ensure we have initialized the context before calling parent code
super.beforeAll()
}
```
Session initialized before thread snapshot. This should also happen in the opposite order. Because I've seen the comment in the code I decided not to change it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84574 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84574/testReport)** for PR 19893 at commit [`1a64209`](https://github.com/apache/spark/commit/1a64209e0208c1f6d1e7a54db2e8cae958f409cf).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
Yeah, I don't think this can be added before figuring out SQL tests.
```
$ grep 'POSSIBLE THREAD LEAK' unit-tests.log | wc -l
158
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84617/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84858/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86093 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86093/testReport)** for PR 19893 at commit [`56a41df`](https://github.com/apache/spark/commit/56a41dfece2513cd31ca10e7f4529d4b524a0814).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158513488
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
--- End diff --
nit:
```
val shortSuiteName = this.getClass.getName.replaceAll("org.apache.spark", "o.a.s")
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84606 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84606/testReport)** for PR 19893 at commit [`62cb32b`](https://github.com/apache/spark/commit/62cb32b9058c3cf3aac4821cb3eecb83d6b577e7).
* This patch **fails to generate documentation**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Related hive please see my comment on 11 Dec 2017.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155601696
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
+ * For more details, see [[org.apache.spark.rpc.RpcEnv]].
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+ "dispatcher-event-loop.*",
+
+ /**
+ * These threads are created by spark when the BlockManager's BlockTransferService initialized
--- End diff --
Same applies here like RPC related threads. Drop them?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156902350
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+// scalastyle:off
+import scala.collection.JavaConversions._
--- End diff --
Yeah, now I see that's not the custom in other files. Thanks for the hint. Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159955747
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,22 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Auto thread audit is turned off here intentionally and done manually.
--- End diff --
I'm still a little not convinced that this is needed.
I still think that any reported leaks here are caused by bugs in the test suites and not because of this. The code you have here is basically the same thing as `SparkFunSuite`.
For example, if a suite extending this does not call `super.beforeAll()` but calls `super.afterAll()`, won't you get false positives in the output?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155592359
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
+ * For more details, see [[org.apache.spark.rpc.RpcEnv]].
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+ "dispatcher-event-loop.*",
+
+ /**
+ * These threads are created by spark when the BlockManager's BlockTransferService initialized
--- End diff --
Same, why is it ok to whitelist them?
I know what these threads are from their names. I just don't know why they should stay running.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155639843
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
+ * For more details, see [[org.apache.spark.rpc.RpcEnv]].
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+ "dispatcher-event-loop.*",
+
+ /**
+ * These threads are created by spark when the BlockManager's BlockTransferService initialized
--- End diff --
Removed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
> All in all if you have an approach which solves context reuse feel free to share.
I don't have one, but it feels sub-optimal to add code that will knowingly trigger false positives in a large number of existing test suites, so it should probably be handled somehow.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #4006 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4006/testReport)** for PR 19893 at commit [`0d45a5b`](https://github.com/apache/spark/commit/0d45a5b7a225e33eedf13ee1f379ea64d6f53c18).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/19893
hi I'm just getting back, thanks for looking at this -- one quick comment on the discussion about why there needs to be a whitelist at all. I have a vague memory of looking at this a while back, and that even when you properly stop netty, all the cleanup is not done immediately, those threads linger around briefly for some reason. I dont' recall exactly which set of threads that applies to.
anyway sounds like you have already figured this out independently, but just wanted to add my findings.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
I've analysed the hive related test flow and found SparkSession and SQLContext sharing between suites as you mentioned. Here is the execution flow:
1. The first hive test suite instantiates TestHive which creates SparkSession and SQLContext
2. SparkFunSuite.beforeAll creates a thread snapshot
3. Test code runs
4. TestHiveSingleton.afterAll resets SparkSession
5. SparkFunSuite.afterAll prints out the possible leaks
Step one executed only by the first hive suite and never again.
Here I do not see false positives in big scale. The only possible false positive threads what I foresee could come from lazy initialisation within SparkSession or SQLContext. On the leftover side we're not tracking SparkSession and SQLContext threads but because of the singleton nature my suggestion is to leave it like that.
In this case you mentioned
```
$ grep 'POSSIBLE THREAD LEAK' unit-tests.log | wc -l
158
```
I can imagine the following situations:
1. Test doesn't call hiveContext.reset()
2. Test creates thread but not frees up
3. Production code issue
4. ...
Of course there could be other issues which I've not considered, please share your ideas.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85722/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@vanzin @squito @srowen @jiangxb1987 @henryr
Big thanks to everybody for the constructive comments, learned a lot from them.
I'll take a look at further possibilities like the suggested spark-hive module.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84606 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84606/testReport)** for PR 19893 at commit [`62cb32b`](https://github.com/apache/spark/commit/62cb32b9058c3cf3aac4821cb3eecb83d6b577e7).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
cc @squito @srowen @HyukjinKwon
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155383597
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,24 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ "rpc-client.*", "rpc-server.*", "shuffle-client.*", "shuffle-server.*",
--- End diff --
For the new netty related patterns added documentation. Could somebody help me out with rpc and shuffle? All I can see for example TaskSetManagerSuite.test("TaskSet with no preferences") creates a lot of them and I don't see any test issue.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85802 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85802/testReport)** for PR 19893 at commit [`87c4852`](https://github.com/apache/spark/commit/87c48520ef7c3236c217cdfd787e048866fcb75e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155609975
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
`TaskSetManagerSuite` extends `LocalSparkContext` which stops contexts in its `afterEach` method, so if the context is not being stopped, or the threads are not being stopped, there's an issue somewhere.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86050 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86050/testReport)** for PR 19893 at commit [`9c9c6ef`](https://github.com/apache/spark/commit/9c9c6ef05e6ed6d1ad1a50591a0ed84811876bd7).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159955132
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
--- End diff --
I'd just remove this paragraph since this class is independent of `SparkFunSuite`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r154956887
--- Diff: core/src/test/resources/thread_whitelist ---
@@ -0,0 +1,24 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Each line contains a new regex string which will be evaluated with matches
+# Empty lines or starting with # will me skipped
+
+rpc-client.*
--- End diff --
If this is just for testing support, I personally think there's no need to create a config file and read it. Hard-coding filtering rules may be just fine. Neutral on this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Checked and seems like unrelated.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
On the other side globalEventExecutor.* and dag-scheduler-event-loop was an issue in the tests what I've taken a look at.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85290/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159837804
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SessionStateSuite.scala ---
@@ -39,6 +41,7 @@ class SessionStateSuite extends SparkFunSuite
protected var activeSession: SparkSession = _
override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
You still have not looked at the hive tests.
```
$ grep -a 'POSSIBLE THREAD LEAK' unit-tests.log | wc -l
61
```
A bunch of those look like false positives.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86035 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86035/testReport)** for PR 19893 at commit [`9c9c6ef`](https://github.com/apache/spark/commit/9c9c6ef05e6ed6d1ad1a50591a0ed84811876bd7).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
gentle ping @jiangxb1987
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85315 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85315/testReport)** for PR 19893 at commit [`f7939fa`](https://github.com/apache/spark/commit/f7939fab92a205168c9713e371d490f5c036577e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84574 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84574/testReport)** for PR 19893 at commit [`1a64209`](https://github.com/apache/spark/commit/1a64209e0208c1f6d1e7a54db2e8cae958f409cf).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155602584
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
> These can be destroyed when sc.stop() called but I've seen this pattern rarely.
Not sure I understand. Tests should be stopping their contexts (sql and hive tests notwithstanding), and if these threads are not going away when that happens, it's a bug.
After all, that's the kind of thing I expect this code to be catching.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19893
Also, what happens when you run tests in the sql and hive modules? Those try to keep the same session alive for multiple suites, so I'd expect a bunch of threads to trigger this code when they really should not, in that case.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by smurakozi <gi...@git.apache.org>.
Github user smurakozi commented on the issue:
https://github.com/apache/spark/pull/19893
Logging the leaked threads in a more grep friendly format would be nice, you could easily create a thread leak report.
It would be also nice to see the leaks on the console.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84601/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by henryr <gi...@git.apache.org>.
Github user henryr commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155359296
--- Diff: core/src/test/resources/thread_whitelist ---
@@ -0,0 +1,24 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# Each line contains a new regex string which will be evaluated with matches
+# Empty lines or starting with # will me skipped
+
+rpc-client.*
--- End diff --
Agree with Sean here - there's not a really obvious use case for having this independent of the class where it's used. Putting it into the code means that the whitelist feature is self-documenting, and you don't have to go through any indirection to find this file.
Plus I think moving this into SparkFunSuite means you can get rid of the file loading logic in 'object SparkFunSuite'.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161370006
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
Much better phrased and compressed explanation, applied. Agree that it would be better to move this functionality into `SharedSparkSession` on the other hand it would lead far in terms of number of modifications. `SharedSparkSession` has to extend has to extend `SparkFunSuite` which I don't see it worth the effort. The other option what I see also doesn't help in terms of understanding. Namely moving manual thread audit into `SparkFunSuite` and leaving `enableAutoThreadAudit = false` in `SharedSQLContext` but splitting functionality rarely can help. Ideas?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156902396
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+// scalastyle:off
+import scala.collection.JavaConversions._
+
+import org.apache.spark.internal.Logging
+// scalastyle:on
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
+ threadNamesSnapshot = runningThreadNames
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val suiteName = this.getClass.getName
+ val shortSuiteName = suiteName.replaceAll("org.apache.spark", "o.a.s")
+
+ if (threadNamesSnapshot.nonEmpty) {
+ val remainingThreadNames = runningThreadNames.diff(threadNamesSnapshot)
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ if (remainingThreadNames.nonEmpty) {
+ logWarning(s"\n\n===== POSSIBLE THREAD LEAK IN SUITE $shortSuiteName, " +
+ s"thread names: ${remainingThreadNames.mkString(", ")} =====\n")
+ }
+ } else {
+ logWarning(s"\n\n===== THREAD AUDIT POST ACTION CALLED " +
+ s"WITHOUT PRE ACTION IN SUITE $shortSuiteName =====\n")
+ }
+ }
+
+ private def runningThreadNames(): Set[String] = {
+ Thread.getAllStackTraces.keySet().map(_.getName).toSet
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Good point, I've also struggled to collect all actual problems :)
Format changed to the following:
```
===== FINISHED o.a.s.scheduler.DAGSchedulerSuite: 'task end event should have updated accumulators (SPARK-20342)' =====
...
ScalaTest-run:
===== Global thread whitelist loaded with name /thread_whitelist from classpath: rpc-client.*, rpc-server.*, shuffle-client.*, shuffle-server.*' =====
ScalaTest-run:
===== POSSIBLE THREAD LEAK IN SUITE o.a.s.scheduler.DAGSchedulerSuite, thread names: globalEventExecutor-2-6 =====
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by henryr <gi...@git.apache.org>.
Github user henryr commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155358158
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
--- End diff --
Suggest turning this into runningThreadNames(): Set[String], and then you can use this method both in beforeAll() and in printRemainingThreadNames() (line 70). And you can maybe put the whitelist logic here as well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84574/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161319704
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
Ok, I think I see your point. Still, the comment here is confusing. Can't this be done in `SharedSparkSession` instead, where that initialization happens, so that it's clear what it's talking about?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
In the meantime analysed a couple of cases and found netty related threads:
- netty.*
- globalEventExecutor.*
- threadDeathWatcher.*
I've added them to the whitelist.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Here is a list but it definitely contains false positives.
[SPARK-16139.txt](https://github.com/apache/spark/files/1535397/SPARK-16139.txt)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84763 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84763/testReport)** for PR 19893 at commit [`3bde33c`](https://github.com/apache/spark/commit/3bde33c481a35257497a1b9d4787bdc9ab7c7a3c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `trait ThreadAudit extends Logging `
* `trait SharedSQLContext extends SQLTestUtils with SharedSparkSession `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #85315 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85315/testReport)** for PR 19893 at commit [`f7939fa`](https://github.com/apache/spark/commit/f7939fab92a205168c9713e371d490f5c036577e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159524703
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
--- End diff --
Can't you just inline this in `doThreadPreAudit` since it's the only call site and this is a private method?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155366150
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
+ filterNot(s => SparkFunSuite.threadWhiteList.exists(s.matches(_)))
--- End diff --
style: `.filterNot { s => `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155606762
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -34,12 +36,53 @@ abstract class SparkFunSuite
with Logging {
// scalastyle:on
+ val threadWhiteList = Set(
+ /**
+ * Netty related threads.
+ */
+ "netty.*",
+
+ /**
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty creates such threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * These threads are created by spark when internal RPC environment initialized and later used.
--- End diff --
Sounds like the case (removing from whitelist), but if tests really weren't stopping their contexts, I'd expect other errors to happen. Do you have examples of tests that show this issue?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159832598
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import scala.collection.JavaConverters._
+
+import org.apache.spark.internal.Logging
+
+/**
+ * Thread audit for test suites.
+ *
+ * Thread audit happens normally in [[SparkFunSuite]] automatically when a new test suite created.
+ * The only prerequisite for that is that the test class must extend [[SparkFunSuite]].
+ *
+ * There are some test suites which are doing initialization before [[SparkFunSuite#beforeAll]]
+ * executed. This case auditing can be moved into another place in the call sequence.
+ *
+ * To do the audit in a custom place/way the following can be done:
+ *
+ * class MyTestSuite extends SparkFunSuite {
+ *
+ * override val doThreadAuditInSparkFunSuite = false
+ *
+ * protected override def beforeAll(): Unit = {
+ * doThreadPreAudit
+ * super.beforeAll
+ * }
+ *
+ * protected override def afterAll(): Unit = {
+ * super.afterAll
+ * doThreadPostAudit
+ * }
+ * }
+ */
+trait ThreadAudit extends Logging {
+
+ val threadWhiteList = Set(
+ /**
+ * Netty related internal threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "netty.*",
+
+ /**
+ * Netty related internal threads.
+ * A Single-thread singleton EventExecutor inside netty which creates such threads.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "globalEventExecutor.*",
+
+ /**
+ * Netty related internal threads.
+ * Checks if a thread is alive periodically and runs a task when a thread dies.
+ * These are excluded because their lifecycle is handled by the netty itself
+ * and spark has no explicit effect on them.
+ */
+ "threadDeathWatcher.*",
+
+ /**
+ * During [[SparkContext]] creation [[org.apache.spark.rpc.netty.NettyRpcEnv]]
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "rpc-client.*",
+ "rpc-server.*",
+
+ /**
+ * During [[SparkContext]] creation BlockManager
+ * creates event loops. One is wrapped inside
+ * [[org.apache.spark.network.server.TransportServer]]
+ * the other one is inside [[org.apache.spark.network.client.TransportClient]].
+ * The thread pools behind shut down asynchronously triggered by [[SparkContext#stop]].
+ * Manually checked and all of them stopped properly.
+ */
+ "shuffle-client.*",
+ "shuffle-server.*"
+ )
+ private var threadNamesSnapshot: Set[String] = Set.empty
+
+ protected def doThreadPreAudit(): Unit = snapshotRunningThreadNames
+ protected def doThreadPostAudit(): Unit = printRemainingThreadNames
+
+ private def snapshotRunningThreadNames(): Unit = {
--- End diff --
Inlined.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r161317670
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,37 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ /**
+ * Suites extending [[SharedSQLContext]] are sharing resources (eg. SparkSession) in their tests.
+ * Such resources are initialized by the suite before thread audit takes thread snapshot and
--- End diff --
Why is the latter a problem? At worst you'll have less thread after the suite finishes than when it started, which should be fine, no? The problem is having leaked threads, not the other way around.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r159527436
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SessionStateSuite.scala ---
@@ -39,6 +41,7 @@ class SessionStateSuite extends SparkFunSuite
protected var activeSession: SparkSession = _
override def beforeAll(): Unit = {
+ doThreadPreAudit()
--- End diff --
Isn't the problem here that this is not calling `super.beforeAll()`? If you do that, you don't need to override `doThreadAuditInSparkFunSuite` nor call `doThreadPostAudit` below.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@squito thanks for sharing your findings, it's helpful. Yeah, slowly digging into the deep and finding out what these threads are.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@vanzin I've fixed the problematic tests and added a codepart in the ThreadAudit to highlight such situations. After the build we can see more.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by henryr <gi...@git.apache.org>.
Github user henryr commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155383923
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +66,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def runningThreadNames(): Set[String] = {
+ Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = runningThreadNames()
+ val whitelistedThreadNames = currentThreadNames
+ .filterNot { s => threadWhiteList.exists(s.matches(_)) }
+ val remainingThreadNames = whitelistedThreadNames.diff(beforeAllTestThreadNames)
--- End diff --
nit: I think this would be better written as:
val remainingThreadNames = runningThreadNames.diff(beforeAllTestThreadNames).filterNot { s => threadWhiteList.exists(s.matches(_)) }
(although putting the whitelist filtering into runningThreadNames() would still make this more concise).
The reason is that it's not obvious to the reader why you whitelist 'after' threads but not 'before' - clearer to whitelist the diff.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155393714
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -20,10 +20,12 @@ package org.apache.spark
// scalastyle:off
import java.io.File
-import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
-
import org.apache.spark.internal.Logging
import org.apache.spark.util.AccumulatorContext
+import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
+
+import scala.collection.JavaConversions._
--- End diff --
I don't know what that is.
The import order is described in http://spark.apache.org/contributing.html, section "Imports".
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155381411
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -20,10 +20,12 @@ package org.apache.spark
// scalastyle:off
import java.io.File
-import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
-
import org.apache.spark.internal.Logging
import org.apache.spark.util.AccumulatorContext
+import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
+
+import scala.collection.JavaConversions._
--- End diff --
I've executed reorganize imports. Shouldn't solve this such problems?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155370694
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
+ filterNot(s => SparkFunSuite.threadWhiteList.exists(s.matches(_)))
--- End diff --
Style applied.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r158527262
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,4 +17,17 @@
package org.apache.spark.sql.test
-trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
+
+ override protected val doThreadAuditInSparkFunSuite = false
+
+ protected override def beforeAll(): Unit = {
+ doThreadPreAudit
+ super.beforeAll
--- End diff --
Fixed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84879 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84879/testReport)** for PR 19893 at commit [`d38a0c6`](https://github.com/apache/spark/commit/d38a0c607d6a6d4a3e941115fae2db298adefa6e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84580 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84580/testReport)** for PR 19893 at commit [`fe6cd0c`](https://github.com/apache/spark/commit/fe6cd0cc727a9d4516f6af674e616f0b1f8cef93).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155511923
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -20,10 +20,12 @@ package org.apache.spark
// scalastyle:off
import java.io.File
-import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
-
import org.apache.spark.internal.Logging
import org.apache.spark.util.AccumulatorContext
+import org.scalatest.{BeforeAndAfterAll, FunSuite, Outcome}
+
+import scala.collection.JavaConversions._
--- End diff --
Thanks for the guidance. I've set up the intellij imports organizer as described and fixed with it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84580/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Thread audit disabled in hive.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
The last suspicious big group of threads (at least for me) is broadcast-exchange.* but as I've seen this is not false positive because the threadpool never stopped. In BroadcastExchangeExec:141 a new daemon thread pool created in case of broadcast join which dies when jvm dies.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/19893
> The last suspicious big group of threads (at least for me) is broadcast-exchange.* but as I've seen this is not false positive because the threadpool never stopped. In BroadcastExchangeExec:141 a new daemon thread pool created in case of broadcast join which dies when jvm dies. I suggest to solve this separately.
what do you mean about handling this separately? should it be whitelisted for now, or should there be another jira for it? It seems fine to me to set it aside for now, just want to be clear what the next steps are.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155370856
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -72,3 +99,27 @@ abstract class SparkFunSuite
}
}
+
+object SparkFunSuite
+ extends Logging {
--- End diff --
Object removed due to previous review items.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84869 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84869/testReport)** for PR 19893 at commit [`827b6dc`](https://github.com/apache/spark/commit/827b6dca818d3f678c74f0edda04a1f9b315f932).
* This patch **fails to generate documentation**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
@jiangxb1987 feel free to take a look at it. More eyes, more possibilities.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84617 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84617/testReport)** for PR 19893 at commit [`644ee6a`](https://github.com/apache/spark/commit/644ee6aec14faa89789dd9ef52fb539e93a7076d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155366220
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
+ filterNot(s => SparkFunSuite.threadWhiteList.exists(s.matches(_)))
+ val remainingThreadNames = whitelistedThreadNames.diff(beforeAllTestThreadNames)
+ if (!remainingThreadNames.isEmpty) {
--- End diff --
`remainingThreadNames.nonEmpty`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86093/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
Still to come. I'll put hive findings here the same way.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r156803613
--- Diff: core/src/test/scala/org/apache/spark/ThreadAudit.scala ---
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+// scalastyle:off
+import scala.collection.JavaConversions._
--- End diff --
don't turn the scalastyle warning off here -- do what the warning says, switch to importing `JavaConverters._` instead
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84879 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84879/testReport)** for PR 19893 at commit [`d38a0c6`](https://github.com/apache/spark/commit/d38a0c607d6a6d4a3e941115fae2db298adefa6e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by gaborgsomogyi <gi...@git.apache.org>.
Github user gaborgsomogyi commented on the issue:
https://github.com/apache/spark/pull/19893
I've taken a look at the failed test but seems like unrelated.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19893
Do any of those leaked threads look like they might be real issues to fix? you could paste the results here, minus anything you know isn't a problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/19893
The `TestHiveContext` is shared among test suites, maybe it's not a good time to change this for now. Could we create a new class that inherit from `SparkFunSuite` that examines leaking threads, and only have test suites in core module extend from this new class? WDYT @gatorsmile @cloud-fan ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19893: [SPARK-16139][TEST] Add logging functionality for...
Posted by henryr <gi...@git.apache.org>.
Github user henryr commented on a diff in the pull request:
https://github.com/apache/spark/pull/19893#discussion_r155358362
--- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala ---
@@ -52,6 +62,23 @@ abstract class SparkFunSuite
getTestResourceFile(file).getCanonicalPath
}
+ private def saveThreadNames(): Unit = {
+ beforeAllTestThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ }
+
+ private def printRemainingThreadNames(): Unit = {
+ val currentThreadNames = Thread.getAllStackTraces.keySet().map(_.getName).toSet
+ val whitelistedThreadNames = currentThreadNames.
--- End diff --
nit: '.' goes on next line
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #86050 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86050/testReport)** for PR 19893 at commit [`9c9c6ef`](https://github.com/apache/spark/commit/9c9c6ef05e6ed6d1ad1a50591a0ed84811876bd7).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19893
Build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19893: [SPARK-16139][TEST] Add logging functionality for leaked...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19893
**[Test build #84915 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84915/testReport)** for PR 19893 at commit [`ef00796`](https://github.com/apache/spark/commit/ef007969a576dd5f84440ce78a325d6617dc5278).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org