-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-9886][CORE] Fix to use ShutdownHookManager in #10313
Conversation
ExternalBlockStore.scala
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. |
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? |
Hi @JoshRosen, Are you suggesting to go ahead look over "Runtime.getRuntime.addShutdownHook" use and get rid of same |
@naveenminchu, yeah, I'm suggesting that we add a rule to Line 160 in bc1ff9f
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. |
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. |
Test build #2218 has finished for PR 10313 at commit
|
Merging into master 1.6. We can add the scalastyle check later. |
ExternalBlockStore.scala Author: Naveen <naveenminchu@gmail.com> Closes #10313 from naveenminchu/branch-fix-SPARK-9886. (cherry picked from commit 8a215d2) Signed-off-by: Andrew Or <andrew@databricks.com>
There're still 3 references to Runtime.getRuntime.addShutdownHook() in the code base. |
I believe so, and we can also add a scalastyle check that Josh suggested |
Created #10325 |
@tedyu Thanks for making those changes |
ExternalBlockStore.scala