Uh oh!
There was an error while loading. Please reload this page.
SPARK-19794 Release HDFS Client after read/write checkpoint - #17135
SPARK-19794 Release HDFS Client after read/write checkpoint#17135darionyaphet wants to merge 1 commit into
Conversation
srowen
commented
Mar 2, 2017
I get the idea, but I'm not sure any of these are valid |
| logInfo(s"Final output path $finalOutputPath already exists; not overwriting it") | ||
| if (!fs.delete(tempOutputPath, false)) { | ||
| logWarning(s"Error deleting ${tempOutputPath}") | ||
| try { |
There was a problem hiding this comment.
Given that this doesn't encompass the span of usage for fs -- better to just call fs.close() at the end and not worry about manually closing in an error case? or expand the try-finally?
Actually, I am not sure we are supposed to call FileSystem.close() because they are shared instances, cached and reused across the whole application.
There was a problem hiding this comment.
Agreed with @srowen , FileSystem is a cached object, closing it means removed it from cache. I don't think we need to call this explicitly. Because by default it is designed to be shared.
| serializeStream.writeObject(partitioner) | ||
| } { | ||
| serializeStream.close() | ||
| fileOutputStream.close() |
There was a problem hiding this comment.
Ditto, this is OK if serializeStream.close() doesn't actually close the underlying stream (?) but not sure about the next line.
| context.addTaskCompletionListener(context => deserializeStream.close()) | ||
| deserializeStream.asIterator.asInstanceOf[Iterator[T]] | ||
| Utils.tryWithSafeFinally { |
There was a problem hiding this comment.
I don't think you can close it here, right? you're returning an iterator on the stream
There was a problem hiding this comment.
This code will introduce issue, deserializaStream should be called after finished, the code here will close this stream prematurely.
Also please look at L289, it already takes care of close after the task is finished.
SparkQA
commented
Mar 2, 2017
Test build #3592 has finished for PR 17135 at commit
|
I remember FileSystem will be cached internally by default. Closing it probably will introduce some performance regression. Did you see any case that FileSystem cache doesn't work properly? |
srowen
commented
Mar 3, 2017
Yes this is substantially not something we can merge, so let's close this. |
vanzin
commented
Mar 3, 2017
It's not just a matter of performance regression - it will brake any other code that has references to the file system being closed. -1. |
Closesapache#16819Closesapache#13467Closesapache#16083Closesapache#17135Closesapache#8785Closesapache#16278Closesapache#16997Closesapache#17073Closesapache#17220
Closesapache#16819Closesapache#13467Closesapache#16083Closesapache#17135Closesapache#8785Closesapache#16278Closesapache#16997Closesapache#17073Closesapache#17220 Added: Closesapache#12059Closesapache#12524Closesapache#12888Closesapache#16061 Author: Sean Owen <sowen@cloudera.com> Closesapache#17386 from srowen/StalePRs.
What changes were proposed in this pull request?
Close HDFS client and streams after reading and writing from HDFS .
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review http://spark.apache.org/contributing.html before opening a pull request.