[SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit tests by HyukjinKwon · Pull Request #15618 · apache/spark

HyukjinKwon · 2016-10-25T05:52:38Z

What changes were proposed in this pull request?

Close FileStreams, ZipFiles etc to release the resources after using. Not closing the resources will cause IO Exception to be raised while deleting temp files.

How was this patch tested?

Existing tests

Helping script for windows to download dependency and start zinc to support incremental building on windows.

HyukjinKwon · 2016-10-25T05:54:58Z

This PR simply rebases #12693 because it seems almost there or already done but was not rebased and It has been stale.

I guess this should be credited to @taoli91.

cc @tdas @srowen

SparkQA · 2016-10-25T10:07:32Z

Test build #67491 has finished for PR 15618 at commit f3713d1.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-10-25T10:38:38Z

retest this please

SparkQA · 2016-10-25T12:34:17Z

Test build #67507 has finished for PR 15618 at commit f3713d1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-10-25T14:58:06Z

retest this please

HyukjinKwon · 2016-10-25T15:05:12Z

Let me take a look into this deeper if some tests constantly fail.

SparkQA · 2016-10-25T17:10:13Z

Test build #67516 has finished for PR 15618 at commit f3713d1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mridulm · 2016-10-25T17:24:30Z

There are a bunch of methods in Utils which actually nicely apply to this PR.

Utils.tryWithSafeFinally
Utils.tryWithResource, etc

They also handle exception suppression, etc.

felixcheung · 2016-10-25T22:10:38Z

+ serializer.deserializeStream(fileInputStream)
+ } catch {
+ case e : Throwable =>
+ fileInputStream.close()


shouldn't this be in a try {} finally {} instead?

Not here because the stream needs to be open afterwards. I had a similar discussion on the original pr.

I don't mean having the finally here on this line...

I am sorry to ask repeatedly but could I please ask how I should change this a little bit more?
I was thinking I should not use finally upon fileInputStream as it should not always be closed at this point or around this point..

Ah, did you mean something like this?
valpartitioner=Utils.tryWithSafeFinally[Partitioner] { valdeserializeStream= serializer.deserializeStream(fileInputStream) Utils.tryWithSafeFinally[Partitioner] { deserializeStream.readObject[Partitioner] } { deserializeStream.close() } } { fileInputStream.close() }

Yes I think that's a good idea. Do you need the generic types on the "try" methods, even?

Ah-ha, sure. Let me then try. Thanks for bearing with me.

HyukjinKwon · 2016-10-26T00:39:59Z

Thanks @mridulm and @felixcheung. Let me try to address your comments.

HyukjinKwon · 2016-10-26T01:07:18Z

cc @fuzhouch too (we talked about this via emails).

HyukjinKwon · 2016-10-26T02:04:59Z

I just took a look for Utils.tryWithSafeFinally and Utils.tryWithResource. It seems both Utils.tryWithSafeFinally and Utils.tryWithResource will close it in finally if I understand this correctly. Could I just leave as it is?

mridulm · 2016-10-26T03:56:19Z

@HyukjinKwon So the idea is that you acquire resources required and dont need to track it by wrapping them in Utils.tryWithResource (similar to memory management in jvm).

As an example:
main/scala/org/apache/spark/rdd/ReliableCheckpointRDD.scala change will simply acquire the fileInputStream in the try and release it in the finally automatically - without needing to manage it via catch/rethrow, etc (ex: what if close() throws exception ?).

Even core/src/test/scala/org/apache/spark/FileSuite.scala, core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala, etc change can be modelled the same way.
You get the idea :-)

This is essentially analogous to try-with-resources in java.
Which is not to say it applies every where ofcourse : drawback is that unlike in java, you need to explicitly specify the finally action, which can be pita imo compared to java's idiom.
And potentially multiple levels of try blocks ...

Since you are anyway going through the pain of making all these changes to fix up code, might be a good idea to change it such that future tests will follow the same pattern.
Thoughts ?

srowen · 2016-10-26T10:15:10Z

It makes sense to use the Utils methods where possible, sure.

…r checks in methods

SparkQA · 2016-11-01T17:39:30Z

Test build #67903 has finished for PR 15618 at commit 1521572.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-11-01T22:32:13Z

retest this please

HyukjinKwon · 2016-11-01T22:34:14Z

Oh wait, it seems the failed test is potentislly related. Will take a look.

SparkQA · 2016-11-02T00:26:46Z

Test build #67928 has finished for PR 15618 at commit 1521572.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2016-11-06T09:01:08Z

Hm.. @srowen @tdas and @mridulm . I just got rid of the changes in ReceiverTracker.scala here for now. It seems we can't make this null in stop() and create a new ReceivedBlockTracker instance in start() because this instance is being accessed after gracefully stopped. It seems the blocks should be able to be retrieved after stopped.

One way I am thinking is, initiate this ReceivedBlockTracker instance at start() in ReceiverTracker and add null checks in each method using ReceivedBlockTracker instance in ReceiverTracker. However, I am not sure whether it is worth adding such checks and that this is right or cleanest way.

Otherwise, could I please ask anyone who is familiar with this to make a followup if it is fine?

SparkQA · 2016-11-06T09:57:28Z

Test build #68231 has finished for PR 15618 at commit a9a5f06.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

OK perhaps except for one last question.

srowen · 2016-11-06T14:23:03Z


 override def beforeAll(): Unit = {
 super.beforeAll()
+ checkpointDir = Utils.createTempDir("checkpoint")


This might happen to be OK, but now the dir is not deleted between tests. Is that going to be OK?

Actually, it seems the original codes are already fine in here.. I took a look and ran several tests and it seems now I may understand why @taoli91 tried to fix the codes like this.
It seems somehow the state in ReceiverTracker went wrong on Windows and ended up without closing checkpointDir. It seems the original test was failed[1] due to the issue in ReceiverTracker. So, I think he tried to only create/delete the folder once[2] and ensure stopping ReceivedBlockTracker in ReceiverTracker regardless of the state.
I tested this with manually stopping ReceivedBlockTracker regardless of the state (the original proposal) and it seems fine without the changes in here, MapWithStateSuite.scala[3]. Of course, it is fine with this change[4].
[1]https://ci.appveyor.com/project/spark-test/spark/build/56-F88EDDAF-E576-4787-9530-A4185FC46B1E
[2]https://ci.appveyor.com/project/spark-test/spark/build/57-test-MapWithStateSuite
[3]https://ci.appveyor.com/project/spark-test/spark/build/58-test-MapWithStateSuite
[4]https://ci.appveyor.com/project/spark-test/spark/build/59-test-MapWithStateSuite

I believe this change is not valid. I will get rid of this. Thank you for pointing this out.

BTW, the reason why [1][2] were failed on Windows (without ensuring stopping ReceivedBlockTracker) seems, the directory, checkpointDir, is being opened so it fails to delete the directory throwing an exception.

SparkQA · 2016-11-07T15:32:56Z

Test build #68273 has finished for PR 15618 at commit d680a2f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mridulm · 2016-11-07T20:40:55Z

LGTM, merging it into master.
Thx @HyukjinKwon

srowen · 2016-11-10T10:17:47Z

I'm going to merge to 2.1 as well to match #15320

srowen · 2016-11-10T10:55:06Z

Scratch that; it doesn't merge cleanly. Let's leave it.

…ts and example ## What changes were proposed in this pull request? This is a follow-up work of apache#15618. Close file source; For any newly created streaming context outside the withContext, explicitly close the context. ## How was this patch tested? Existing unit tests. Author: wm624@hotmail.com <wm624@hotmail.com> Closesapache#15818 from wangmiao1981/rtest.

…nit tests ## What changes were proposed in this pull request? Close `FileStreams`, `ZipFiles` etc to release the resources after using. Not closing the resources will cause IO Exception to be raised while deleting temp files. ## How was this patch tested? Existing tests Author: U-FAREAST\tl <tl@microsoft.com> Author: hyukjinkwon <gurwls223@gmail.com> Author: Tao LI <tl@microsoft.com> Closesapache#15618 from HyukjinKwon/SPARK-14914-1.

…ts and example ## What changes were proposed in this pull request? This is a follow-up work of apache#15618. Close file source; For any newly created streaming context outside the withContext, explicitly close the context. ## How was this patch tested? Existing unit tests. Author: wm624@hotmail.com <wm624@hotmail.com> Closesapache#15818 from wangmiao1981/rtest.

taoli91and others added 15 commits October 25, 2016 14:28

mvn.cmd for windows
ceb1681
Helping script for windows to download dependency and start zinc to support incremental building on windows.

Fix file not closed on FileSuite
98ee4ab

Fix another file closing
ed52ec7

close log data.
448508f

Close the class loader
34078a3

Another file not closed.
efb7227

Stop to release resources.
a06bffc

More closing problem
45262dc

Fix the zip file and jar file in RPackageUtilsSuite
b3c0c96

Stop ssc in MasterFailureTest
a176adb

Remove accidentally added files
35aacd2

Code cleanup with respect to comments
9f50128

Style fixes
55b360e

Minor code cleanup
91f82b5

ex -> e and indentation
f3713d1

srowen approved these changes Oct 25, 2016
View reviewed changes

felixcheung reviewed Oct 25, 2016
View reviewed changes

Require nulls for eventpoint and the tracker in start and remove othe…
1521572
…r checks in methods

HyukjinKwon closed this Nov 1, 2016

HyukjinKwon reopened this Nov 1, 2016

HyukjinKwon changed the title ~~[SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit tests~~[WiP][SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit testsNov 1, 2016

HyukjinKwon changed the title ~~[WiP][SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit tests~~[WIP][SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit testsNov 1, 2016

Get rid of the changes in ReceiverTracker
a9a5f06

HyukjinKwon changed the title ~~[WIP][SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit tests~~[SPARK-14914][CORE] Fix Resource not closed after using, mostly for unit testsNov 6, 2016

srowen approved these changes Nov 6, 2016
View reviewed changes

Revert the changes in MapWithStateSuite
d680a2f

srowen approved these changes Nov 7, 2016
View reviewed changes

asfgit closed this in 8f0ea01Nov 7, 2016

wangmiao1981 mentioned this pull request Nov 9, 2016
[SPARK-14914][CORE]:Fix Resource not closed after using, for unit tests and example #15818
Closed

HyukjinKwon mentioned this pull request Nov 12, 2016
[SPARK-18422][CORE] Fix wholeTextFiles test to pass on Windows in JavaAPISuite #15866
Closed

HyukjinKwon deleted the SPARK-14914-1 branch January 2, 2018 03:39

Uh oh!

Conversation

HyukjinKwon commented Oct 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Oct 25, 2016

Uh oh!

SparkQA commented Oct 25, 2016

Uh oh!

HyukjinKwon commented Oct 25, 2016

Uh oh!

SparkQA commented Oct 25, 2016

Uh oh!

HyukjinKwon commented Oct 25, 2016

Uh oh!

HyukjinKwon commented Oct 25, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Oct 25, 2016

Uh oh!

mridulm commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 26, 2016

Uh oh!

HyukjinKwon commented Oct 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Oct 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mridulm commented Oct 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Oct 26, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

HyukjinKwon commented Nov 1, 2016

Uh oh!

HyukjinKwon commented Nov 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 2, 2016

Uh oh!

HyukjinKwon commented Nov 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 6, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonNov 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwonNov 7, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 25, 2016•
edited
Loading

HyukjinKwon commented Oct 25, 2016•
edited
Loading

HyukjinKwon commented Oct 26, 2016•
edited
Loading

HyukjinKwon commented Oct 26, 2016•
edited
Loading

mridulm commented Oct 26, 2016•
edited
Loading

HyukjinKwon commented Nov 1, 2016•
edited
Loading

HyukjinKwon commented Nov 6, 2016•
edited
Loading

HyukjinKwonNov 7, 2016•
edited
Loading

HyukjinKwonNov 7, 2016•
edited
Loading