You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by naveenminchu <gi...@git.apache.org> on 2015/12/15 19:53:58 UTC
[GitHub] spark pull request: [SPARK-9886][CORE] Fix to use ShutdownHookMana...
GitHub user naveenminchu opened a pull request:
https://github.com/apache/spark/pull/10313
[SPARK-9886][CORE] Fix to use ShutdownHookManager in
ExternalBlockStore.scala
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/naveenminchu/spark branch-fix-SPARK-9886
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10313.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 #10313
----
commit 2672ddb298f915c8a3b039c683b3a1b6ee873a36
Author: Naveen <na...@gmail.com>
Date: 2015-12-15T18:48:32Z
[SPARK-9886][CORE] Fix to use ShutdownHookManager in
ExternalBlockStore.scala
----
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-165076390
Created #10325
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-165014078
I believe so, and we can also add a scalastyle check that Josh suggested
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164888522
The comment from @vanzin on the JIRA indicated he wasn't sure whether the other 2 usages were better left alone as they're used in particular stand-alone processes. But I suppose it doesn't really hurt to use the fancy wrapper in those cases too.
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164864825
@naveenminchu, yeah, I'm suggesting that we add a rule to `scalastyle-config.xml` (like https://github.com/apache/spark/blob/bc1ff9f4a41401599d3a87fb3c23a2078228a29b/scalastyle-config.xml#L160), add excludes to ignore the legitimate uses, then fix any violations that we find (see #7350 for an example of this)
I suppose the Scalastyle lint doesn't catch similar problems in Java code, but I say "let's not let perfect be the enemy of the good" here; it's better to have some checks vs. no checks.
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164988105
There're still 4 references to Runtime.getRuntime.addShutdownHook() in the code base.
Should they be replaced with ShutdownHookManager ?
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/10313
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164856021
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by naveenminchu <gi...@git.apache.org>.
Github user naveenminchu commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-165186946
@tedyu Thanks for making those changes
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164856111
LGTM, but not sure how we missed this one. It makes me wonder if there's a particular reason for it. But I don't see a reason the current implementation would guarantee some behavior that's required.
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by naveenminchu <gi...@git.apache.org>.
Github user naveenminchu commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164862829
Hi @JoshRosen, Are you suggesting to go ahead look over "Runtime.getRuntime.addShutdownHook" use and get rid of same
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164857807
Hey, can we add a linter rule to ban uses of "Runtime.getRuntime.addShutdownHook", similar to our existing rule for "Class.forName", which recommends using our own utility function 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-9886][CORE] Fix to use ShutdownHookMana...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164966305
Merging into master 1.6. We can add the scalastyle check later.
---
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-9886][CORE] Fix to use ShutdownHookMana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164892782
**[Test build #2218 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2218/consoleFull)** for PR 10313 at commit [`2672ddb`](https://github.com/apache/spark/commit/2672ddb298f915c8a3b039c683b3a1b6ee873a36).
* 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-9886][CORE] Fix to use ShutdownHookMana...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10313#issuecomment-164856574
**[Test build #2218 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2218/consoleFull)** for PR 10313 at commit [`2672ddb`](https://github.com/apache/spark/commit/2672ddb298f915c8a3b039c683b3a1b6ee873a36).
---
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