[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-10-05 Thread asfgit
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/577 --- 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 enabl

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-10-05 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r18436136 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-10-05 Thread andrewor14
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-57943959 Thanks @zsxwing I'm merging this. I'll try to get this into 1.1 as well but there might be merge conflicts. --- If your project is set up for it, you can reply to this

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-10-05 Thread zsxwing
Github user zsxwing commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r18433883 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-17 Thread zsxwing
Github user zsxwing commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17704914 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-17 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17701696 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55843952 [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20443/consoleFull) for PR 577 at commit [`c431095`](https://github.com/apa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55840427 Thank for reviewing. Already fixed the minor problems. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If y

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread zsxwing
Github user zsxwing commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17642519 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55839713 [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20443/consoleFull) for PR 577 at commit [`c431095`](https://github.com/apac

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread JoshRosen
Github user JoshRosen commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55807671 Apart from Andrew's minor comments, this looks good to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17618006 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread andrewor14
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55783745 Minor comments, but LGTM. Do others have more to add? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17617974 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,21 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-16 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55755661 @andrewor14 any further suggestion? --- 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 no

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-13 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55500704 [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20274/consoleFull) for PR 577 at commit [`2de96e5`](https://github.com/apa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-13 Thread JoshRosen
Github user JoshRosen commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55499135 > But I remember that if some file is using, Windows will prevent from deleting it. I'm pretty sure that I observed this issue while trying to run the Maven tes

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-13 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55498864 > Don't know if Windows follows the same semantics, though. Never tested it. But I remember that if some file is using, Windows will prevent from deleting it.

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-13 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55498851 [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20274/consoleFull) for PR 577 at commit [`2de96e5`](https://github.com/apac

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-12 Thread vanzin
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17493894 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,17 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMan

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-12 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55421983 [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20231/consoleFull) for PR 577 at commit [`28b90dc`](https://github.com/apa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-12 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55413623 [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20231/consoleFull) for PR 577 at commit [`28b90dc`](https://github.com/apac

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-12 Thread zsxwing
Github user zsxwing commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17482448 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,17 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-12 Thread zsxwing
Github user zsxwing commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17482426 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,17 @@ private[spark] class DiskStore(blockManager: BlockManager, diskMa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-11 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55347680 [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20177/consoleFull) for PR 577 at commit [`4521d6e`](https://github.com/apa

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-11 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55343435 [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20177/consoleFull) for PR 577 at commit [`4521d6e`](https://github.com/apac

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-11 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17455828 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,17 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-11 Thread andrewor14
Github user andrewor14 commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r17455840 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -73,7 +73,17 @@ private[spark] class DiskStore(blockManager: BlockManager, dis

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-11 Thread andrewor14
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-55343008 retest this please --- 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 fe

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-09-02 Thread JoshRosen
Github user JoshRosen commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-54230446 Jenkins, retest this please. --- 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 hav

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-08-05 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-51298416 QA tests have started for PR 577. This patch merges cleanly. View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18007/consoleFull --- If y

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-08-05 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-51298306 Resolved the conflicts. --- 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

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-08-05 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-51286750 QA results for PR 577:- This patch FAILED unit tests.For more information see test ouptut:https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17977/consoleF

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-08-05 Thread SparkQA
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-51286734 QA tests have started for PR 577. This patch DID NOT merge cleanly! View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17977/consoleFull -

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-06 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-42302718 Thanks for your review. Already updated it. --- 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

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-05 Thread mateiz
Github user mateiz commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r12303556 --- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala --- @@ -325,14 +325,19 @@ object SparkSubmitArguments { def getPropertie

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-05 Thread mateiz
Github user mateiz commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r12303501 --- Diff: core/src/main/scala/org/apache/spark/broadcast/HttpBroadcast.scala --- @@ -159,18 +159,24 @@ private[spark] object HttpBroadcast extends Logging {

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-05 Thread mateiz
Github user mateiz commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r12303524 --- Diff: core/src/main/scala/org/apache/spark/broadcast/HttpBroadcast.scala --- @@ -159,18 +159,24 @@ private[spark] object HttpBroadcast extends Logging {

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-05 Thread mateiz
Github user mateiz commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-42248788 It's a good idea to abstract out try/finally long-term, but for now we can use finally's like here. There are some nice ways to implement automatic closing in Scala: http:

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-03 Thread zsxwing
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-42122270 > will be good to enforce proper resource lifecycle. +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-03 Thread mridulm
Github user mridulm commented on the pull request: https://github.com/apache/spark/pull/577#issuecomment-42110570 Might be good idea to abstract out the try/finally idiom out. @mateiz, any thoughts ? We have a bunch of places where resource cleanup does not happen properly - which

[GitHub] spark pull request: SPARK-1656: Fix potential resource leaks

2014-05-03 Thread mridulm
Github user mridulm commented on a diff in the pull request: https://github.com/apache/spark/pull/577#discussion_r12257197 --- Diff: core/src/main/scala/org/apache/spark/storage/DiskStore.scala --- @@ -77,7 +77,12 @@ private class DiskStore(blockManager: BlockManager, diskManager: