You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by nishkamravi2 <gi...@git.apache.org> on 2015/02/19 08:51:19 UTC
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
GitHub user nishkamravi2 opened a pull request:
https://github.com/apache/spark/pull/4690
SPARK-5841 [CORE] [HOTFIX 2] Memory leak in DiskBlockManager
Continue to see IllegalStateException in YARN cluster mode. Adding a simple workaround for now.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nishkamravi2/spark master_nravi
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4690.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 #4690
----
commit 681b36f5fb63e14dc89e17813894227be9e2324f
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-05-08T07:05:33Z
Fix for SPARK-1758: failing test org.apache.spark.JavaAPISuite.wholeTextFiles
The prefix "file:" is missing in the string inserted as key in HashMap
commit 5108700230fd70b995e76598f49bdf328c971e77
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-03T22:25:22Z
Fix in Spark for the Concurrent thread modification issue (SPARK-1097, HADOOP-10456)
commit 6b840f017870207d23e75de224710971ada0b3d0
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-03T22:34:02Z
Undo the fix for SPARK-1758 (the problem is fixed)
commit df2aeb179fca4fc893803c72a657317f5b5539d7
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-09T19:02:59Z
Improved fix for ConcurrentModificationIssue (Spark-1097, Hadoop-10456)
commit eb663ca20c73f9c467192c95fc528c6f55f202be
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-09T19:04:39Z
Merge branch 'master' of https://github.com/apache/spark
commit 5423a03ddf4d747db7261d08a64e32f44e8be95e
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-10T20:06:07Z
Merge branch 'master' of https://github.com/apache/spark
commit 3bf8fad85813037504189cf1323d381fefb6dfbe
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-16T05:47:00Z
Merge branch 'master' of https://github.com/apache/spark
commit 2b630f94079b82df3ebae2b26a3743112afcd526
Author: nravi <nr...@c1704.halxg.cloudera.com>
Date: 2014-06-16T06:00:31Z
Accept memory input as "30g", "512M" instead of an int value, to be consistent with rest of Spark
commit efd688a4e15b79e92d162073035b03362fcf66f0
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-07-13T00:04:17Z
Merge branch 'master' of https://github.com/apache/spark
commit 2e69f112d1be59951cd32da4127d8b51bfa03338
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-21T23:17:15Z
Merge branch 'master' of https://github.com/apache/spark into master_nravi
commit ebcde10252e6c45169ea086e8426ec9997d46490
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-22T06:44:40Z
Modify default YARN memory_overhead-- from an additive constant to a multiplier (redone to resolve merge conflicts)
commit 1cf2d1ef57ed6d783df06dad36b9505bc74329fb
Author: nishkamravi2 <ni...@gmail.com>
Date: 2014-09-22T08:54:33Z
Update YarnAllocator.scala
commit f00fa311945c1eafa8957eae5c84719521761dcd
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-22T23:06:07Z
Improving logging for AM memoryOverhead
commit c726bd9f707ce182ec8d56ffecf9da87dcdb3091
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T01:19:32Z
Merge branch 'master' of https://github.com/apache/spark into master_nravi
Conflicts:
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala
commit 362da5edfd04bd8bad990fb210a9e11b8494fa62
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T19:56:13Z
Additional changes for yarn memory overhead
commit 42c2c3d18862d3632c20931ecfe2c64883c5febf
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T20:02:49Z
Additional changes for yarn memory overhead issue
commit dac1047995c99f5a2670f934eb8d3a4ad9b532c8
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T21:20:38Z
Additional documentation for yarn memory overhead issue
commit 5ac2ec11629e19030ad5577da1eee2d135cc3d1c
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T21:25:44Z
Remove out
commit 35daa6498048cabb736316e2f19e565c99243b7e
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T21:59:22Z
Slight change in the doc for yarn memory overhead
commit 8f76c8b46379736aeb7dbe1a4d88729424a041f7
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-09-25T22:03:00Z
Doc change for yarn memory overhead
commit 636a9ffeb4a4ae0b941edd849dcbabf38821db53
Author: nishkamravi2 <ni...@gmail.com>
Date: 2014-09-30T18:33:28Z
Update YarnAllocator.scala
commit 5f8f9ede0fda5c7a4f6a411c746a3d893f550524
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-11-19T01:46:58Z
Merge branch 'master' of https://github.com/apache/spark into master_nravi
Conflicts:
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala
commit 9f6583eb92b59e88b0994ef45dd0e1a418d349a6
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-11-19T03:42:25Z
Changes to maxResultSize code (improve error message and add condition to check if maxResultSize > 0)
commit 3e1b616a56cedec0b963b7f813a8e0195c3417e8
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-11-19T06:56:55Z
Modify test for maxResultSize
commit 535295a371b259a4fb1eb2ef01ea9f396aed40dd
Author: nishkamravi2 <ni...@gmail.com>
Date: 2014-11-19T21:56:18Z
Update TaskSetManager.scala
commit 5c9a4cb4eb12eb6cefbf9e9dda42b722abe1d3ed
Author: nishkamravi2 <ni...@gmail.com>
Date: 2014-11-19T21:57:01Z
Update TaskSetManagerSuite.scala
commit b446edc5ce672139564bcbc98217862a2921783b
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2014-11-19T23:30:17Z
Merge branch 'master' of https://github.com/apache/spark into master_nravi
commit 79ea8b418f709232ec6e106d7f58bb11ba86fa4a
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2015-02-19T06:03:18Z
Merge branch 'master' of https://github.com/apache/spark into master_nravi
commit f0d12de3a2179d2ee32bf7ae049223b5ee246795
Author: Nishkam Ravi <nr...@cloudera.com>
Date: 2015-02-19T07:45:31Z
Workaround for IllegalStateException caused by recent changes to BlockManager.stop
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75383633
Thanks I merged this into master 1.3
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75198175
Utils.safeRemoveShutdownHook sounds ok, albeit a bit pretentious :) since a try-catch block alone doesn't quite justify creation of a utility function (unless we have a dozen or more occurrences). I would be perfectly fine with just adding a try-catch block around the other occurrence. The null-pointer check seems unnecessary (and incurs cost), so will delete it.
Also, it seems like inShutDown function is expensive. And should be used only when necessary. I would vote for deleting all its occurrences except for the one in SparkUncaughtExceptionHandler. Unless someone feels strongly otherwise, I can do that cleanup as part of this fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75020545
Agreed. That's why a try-catch block seems enough for this one. Ok with removing logging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75103618
Hey @nishkamravi2 @srowen here's another thought, what if we do
```
if (!Utils.isShutdown) {
Runtime.getRuntime.removeShutdownHook(hook)
}
```
instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75309056
@nishkamravi2 the latest changes look fine to me. I just have a minor question. We can do more refactoring outside of this hot fix and after the 1.3 release.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75362487
We've had releases in the past where a simple SparkPi program would exit with a huge harmless exception that confused many users. I would prefer to keep the changes in this hot fix minimal and change the pattern separately.
Also I just realized this never ran tests, so Jenkins this is ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r24979807
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -148,7 +148,13 @@ private[spark] class DiskBlockManager(blockManager: BlockManager, conf: SparkCon
/** Cleanup local dirs and stop shuffle sender. */
private[spark] def stop() {
// Remove the shutdown hook. It causes memory leaks if we leave it around.
- Runtime.getRuntime.removeShutdownHook(shutdownHook)
+ if(shutdownHook != null) {
+ try {
+ Runtime.getRuntime.removeShutdownHook(shutdownHook)
+ } catch {
+ case e: Exception => None
--- End diff --
Narrow this to `IllegalStateException`, to only squash this particular case, hopefully? I think leaving the `null` check in is OK even if I'm not sure it can happen since the variable is assigned to non-`null` in its constructor. Nit: put a space after `if`. I'll pause a beat for others to weigh in today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75018795
I have that commit. The problem continues to manifest itself. This is a workaround, not a fix at the source. I don't think the fix is trivial.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75213802
I am also OK with just using the try-catch in both places it's needed, rather than make a utility function. I wouldn't remove `Utils.inShutdown`. I assume there was some reason to add it. It's expensive but only used in exception / shutdown paths. At least, I would not do this _here_.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75363816
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25009608
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -148,7 +148,13 @@ private[spark] class DiskBlockManager(blockManager: BlockManager, conf: SparkCon
/** Cleanup local dirs and stop shuffle sender. */
private[spark] def stop() {
// Remove the shutdown hook. It causes memory leaks if we leave it around.
- Runtime.getRuntime.removeShutdownHook(shutdownHook)
+ if(shutdownHook != null) {
--- End diff --
actually, when can `shutdownHook` be null?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75108909
@andrewor14 yeah I like that, although, it works by adding and removing a dummy hook and checking for an exception :) Maybe we can make a Utils method for a `safeRemoveShutdownHook` that just catches the exception? there is one other place in `ExecutorRunner` where this can be used too. I know, that starts to be more than a HOTFIX. I'd be OK either construing this as part of the original JIRA or just making a new one for this slightly different issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75364305
This change looks like the right targeted fix. Waiting on Jenkins.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75019602
Ah right. I was glancing at the number of commits and thought it hadn't been rebased. The problem isn't this shutdown hook but another one in `yarn.ApplicationMaster`. Dang. Well I don't think the problem is a `null` shutdown hook, and I don't think it is an error per se. Maybe it's not even worth logging because it's OK? that is, if we find `close()` is called during shutdown and so we can't remove a hook, just keep going?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25103499
--- Diff: core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala ---
@@ -174,11 +174,8 @@ class NewHadoopRDD[K, V](
}
}
} catch {
- case e: Exception => {
- if (!Utils.inShutdown()) {
- logWarning("Exception in RecordReader.close()", e)
- }
- }
+ case e: Exception =>
+ logWarning("Exception in RecordReader.close()", e)
--- End diff --
ok, I agree.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25098261
--- Diff: core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala ---
@@ -174,11 +174,8 @@ class NewHadoopRDD[K, V](
}
}
} catch {
- case e: Exception => {
- if (!Utils.inShutdown()) {
- logWarning("Exception in RecordReader.close()", e)
- }
- }
+ case e: Exception =>
+ logWarning("Exception in RecordReader.close()", e)
--- End diff --
why this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25098634
--- Diff: core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala ---
@@ -174,11 +174,8 @@ class NewHadoopRDD[K, V](
}
}
} catch {
- case e: Exception => {
- if (!Utils.inShutdown()) {
- logWarning("Exception in RecordReader.close()", e)
- }
- }
+ case e: Exception =>
+ logWarning("Exception in RecordReader.close()", e)
--- End diff --
I think we can back that out if there's any question to keep this limited. @JoshRosen Yes that's where we ended up again, now that it's clear that there are several ways and several places this can happen. It's easiest just to ignore the exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75011202
From a recent run:
15/02/18 22:10:49 WARN util.ShutdownHookManager: ShutdownHook '$anon$3' failed, java.lang.IllegalStateException: Shutdown in progress
java.lang.IllegalStateException: Shutdown in progress
at java.lang.ApplicationShutdownHooks.remove(ApplicationShutdownHooks.java:82)
at java.lang.Runtime.removeShutdownHook(Runtime.java:239)
at org.apache.spark.storage.DiskBlockManager.stop(DiskBlockManager.scala:151)
at org.apache.spark.storage.BlockManager.stop(BlockManager.scala:1208)
at org.apache.spark.SparkEnv.stop(SparkEnv.scala:90)
at org.apache.spark.SparkContext.stop(SparkContext.scala:1376)
at org.apache.spark.deploy.yarn.ApplicationMaster$$anon$3.run(ApplicationMaster.scala:107)
at org.apache.hadoop.util.ShutdownHookManager$1.run(ShutdownHookManager.java:54)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25100986
--- Diff: core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala ---
@@ -174,11 +174,8 @@ class NewHadoopRDD[K, V](
}
}
} catch {
- case e: Exception => {
- if (!Utils.inShutdown()) {
- logWarning("Exception in RecordReader.close()", e)
- }
- }
+ case e: Exception =>
+ logWarning("Exception in RecordReader.close()", e)
--- End diff --
sorry, by "back that out" do you mean revert this change or keep this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75110256
Yeah, having a `Utils.safeRemoveShutdownHook` sounds good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25009313
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -148,7 +148,13 @@ private[spark] class DiskBlockManager(blockManager: BlockManager, conf: SparkCon
/** Cleanup local dirs and stop shuffle sender. */
private[spark] def stop() {
// Remove the shutdown hook. It causes memory leaks if we leave it around.
- Runtime.getRuntime.removeShutdownHook(shutdownHook)
+ if(shutdownHook != null) {
--- End diff --
need space after `if`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by MattWhelan <gi...@git.apache.org>.
Github user MattWhelan commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25010357
--- Diff: core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala ---
@@ -148,7 +148,13 @@ private[spark] class DiskBlockManager(blockManager: BlockManager, conf: SparkCon
/** Cleanup local dirs and stop shuffle sender. */
private[spark] def stop() {
// Remove the shutdown hook. It causes memory leaks if we leave it around.
- Runtime.getRuntime.removeShutdownHook(shutdownHook)
+ if(shutdownHook != null) {
--- End diff --
I don't think it can be. Unless you're calling `stop` before the constructor has finished, which doesn't seem possible.
The docs for `removeShutdownHook` imply that you can pass it any thread, and it'll just return false if that thread isn't currently registered, so even double-calls are unlikely to cause problems there. There's just the IllegalStateException, which could happen if someone else's shutdown hook is calling stop. That's definitely worth protecting against.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75337749
@srowen @andrewor14 I can potentially issue a different PR for the performance issue and back it out from here (it does seem somewhat unrelated). In general, I would vote for discouraging use of this function. Code patterns like these tend to spread-- one could argue checking this condition on several other exit paths in Spark? Also, do we know that if we delete the check we would see noisy stack traces or are we just being careful speculatively?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75016363
No, this was already fixed in https://github.com/apache/spark/commit/49c19fdbad57f0609bbcc9278f9eaa8115a73604 I don't think you have that commit yet here. The problem is not do with it being `null`; I don't think it can be and we shouldn't just let the exception occur.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4690#discussion_r25101678
--- Diff: core/src/main/scala/org/apache/spark/rdd/NewHadoopRDD.scala ---
@@ -174,11 +174,8 @@ class NewHadoopRDD[K, V](
}
}
} catch {
- case e: Exception => {
- if (!Utils.inShutdown()) {
- logWarning("Exception in RecordReader.close()", e)
- }
- }
+ case e: Exception =>
+ logWarning("Exception in RecordReader.close()", e)
--- End diff --
Ah sorry, I mean revert. Although I kind of like the additional follow-on changes, for this PR we should foremost focus on patching up the exception. If there's any question about any extra changes, like the one you raised, I'd favor just reverting that part.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4690
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75308506
I still stand by my original suggestion of simply wrapping the single problematic `removeShutdownHook()` call in a `try` block: https://github.com/apache/spark/pull/4627#issuecomment-74613715. Best to keep things simple for now and avoid introducing new bugs while trying to fix this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75010904
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75366651
[Test build #27808 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/27808/consoleFull) for PR 4690 at commit [`d453197`](https://github.com/apache/spark/commit/d453197693c6ad42c9d01767d7780bbced841cd0).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by nishkamravi2 <gi...@git.apache.org>.
Github user nishkamravi2 commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75011028
@srowen @JoshRosen @mattwhelan
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75363855
[Test build #27808 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/27808/consoleFull) for PR 4690 at commit [`d453197`](https://github.com/apache/spark/commit/d453197693c6ad42c9d01767d7780bbced841cd0).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-5841 [CORE] [HOTFIX 2] Memory leak in Di...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4690#issuecomment-75366654
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27808/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org