-
Notifications
You must be signed in to change notification settings - Fork 28.9k
Super minor: Close inputStream in SparkSubmitArguments #914
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
Conversation
It'd be closed after being GC'd anyway...
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15293/ |
|
Jenkins, retest this please. |
|
Merged build triggered. |
|
Merged build started. |
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does symlink work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isFile() is false for symlinks. It may be more conservative to require !file.isDirectory(), since it seems valid to point to a symlinked config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File.isFile() returns true for symlinks which point to files.
$ touch testfile
$ ln -s testfile testlink
$ scala
scala> new java.io.File("testlink").isFile()
res0: Boolean = true
Additionally, since the docs aren't 100% clear and I couldn't find a solid answer from Google, I checked both the UnixFileSystem and WindowsFileSystem. The former uses stat which resolves symbolic links. The latter will set isFile to true if and only if it is not a directory, so symlinks would be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, my test was silly, as I realize I had it pointed at a directory actually.
|
Ok merging this in master & branch-1.0. Thanks! |
`Properties#load()` doesn't close the InputStream, but it'd be closed after being GC'd anyway... Also changed file.getName to file, because getName only shows the filename. This will show the full (possibly relative) path, which is less confusing if it's not found. Author: Aaron Davidson <[email protected]> Closes #914 from aarondav/tiny and squashes the following commits: db9d072 [Aaron Davidson] Super minor: Close inputStream in SparkSubmitArguments (cherry picked from commit 7d52777) Signed-off-by: Reynold Xin <[email protected]>
`Properties#load()` doesn't close the InputStream, but it'd be closed after being GC'd anyway... Also changed file.getName to file, because getName only shows the filename. This will show the full (possibly relative) path, which is less confusing if it's not found. Author: Aaron Davidson <[email protected]> Closes apache#914 from aarondav/tiny and squashes the following commits: db9d072 [Aaron Davidson] Super minor: Close inputStream in SparkSubmitArguments
`Properties#load()` doesn't close the InputStream, but it'd be closed after being GC'd anyway... Also changed file.getName to file, because getName only shows the filename. This will show the full (possibly relative) path, which is less confusing if it's not found. Author: Aaron Davidson <[email protected]> Closes apache#914 from aarondav/tiny and squashes the following commits: db9d072 [Aaron Davidson] Super minor: Close inputStream in SparkSubmitArguments
Properties#load()doesn't close the InputStream, but it'd be closed after being GC'd anyway...Also changed file.getName to file, because getName only shows the filename. This will show the full (possibly relative) path, which is less confusing if it's not found.