You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by zhichao-li <gi...@git.apache.org> on 2015/04/08 06:39:38 UTC
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
GitHub user zhichao-li opened a pull request:
https://github.com/apache/spark/pull/5407
Fix potential resource leaks in CheckPoint CheckpointWriter and CheckpointReader
The close action should be placed within finally block to avoid the potential resource leaks
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhichao-li/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5407.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5407
----
commit 193c542a488ceb9fc5445208ba5647bd97f3c5d2
Author: lisurprise <zh...@intel.com>
Date: 2015-03-03T05:14:11Z
remove streaming tab while closing streaming context
commit 8281bcb7320721a17a5c97736d3f6dadb1fb24a5
Author: lisurprise <zh...@intel.com>
Date: 2015-03-06T07:47:34Z
clean code
commit db25ed2b0a44144c87732088849a7b20bc8edc7d
Author: lisurprise <zh...@intel.com>
Date: 2015-03-06T07:53:40Z
clean code
commit 31a44faec3b7f5d160a1345fb2f1ff6544d6e8ea
Author: lisurprise <zh...@intel.com>
Date: 2015-03-10T01:46:48Z
add unit test for attaching and detaching new tab
commit 51e6c7febc0bc63721f497712580df6f956048f2
Author: lisurprise <zh...@intel.com>
Date: 2015-03-12T04:12:21Z
move detach method into StreamingTab
commit c329806f30cf82a796a0256c2e08f774b7fdd43b
Author: lisurprise <zh...@intel.com>
Date: 2015-03-12T08:22:00Z
add test for attaching/detaching streaming tab
commit b2a3d527fbee0ef60330a97821f45d5c78b392cb
Author: lisurprise <zh...@intel.com>
Date: 2015-03-27T05:50:17Z
Merge remote-tracking branch 'upstream/master'
commit 0b0fd57e38d0e1cc302b53372d3f69cc81eed9c0
Author: lisurprise <zh...@intel.com>
Date: 2015-04-02T00:43:07Z
Merge remote-tracking branch 'upstream/master'
commit 6ce7d45d36750c7176ad798f09711ec720949328
Author: lisurprise <zh...@intel.com>
Date: 2015-04-08T03:02:58Z
Fix potential resource leaks
----
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90854861
[Test build #29848 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29848/consoleFull) for PR 5407 at commit [`824adb3`](https://github.com/apache/spark/commit/824adb3fceb50fc7d6bef054fcd9402ca82796f9).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-92177966
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30127/
Test PASSed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27944111
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -247,6 +250,7 @@ object CheckpointReader extends Logging {
val compressionCodec = CompressionCodec.createCodec(conf)
checkpointFiles.foreach(file => {
logInfo("Attempting to load checkpoint from file " + file)
+ var ois:ObjectInputStreamWithLoader = null
--- End diff --
White space after `:`
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91417249
[Test build #29995 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29995/consoleFull) for PR 5407 at commit [`ef862d6`](https://github.com/apache/spark/commit/ef862d64ee129ffe0f74949147ec0c2576bbd5be).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r28050423
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -248,18 +253,23 @@ object CheckpointReader extends Logging {
checkpointFiles.foreach(file => {
logInfo("Attempting to load checkpoint from file " + file)
try {
- val fis = fs.open(file)
- // ObjectInputStream uses the last defined user-defined class loader in the stack
- // to find classes, which maybe the wrong class loader. Hence, a inherited version
- // of ObjectInputStream is used to explicitly use the current thread's default class
- // loader to find and load classes. This is a well know Java issue and has popped up
- // in other places (e.g., http://jira.codehaus.org/browse/GROOVY-1627)
- val zis = compressionCodec.compressedInputStream(fis)
- val ois = new ObjectInputStreamWithLoader(zis,
- Thread.currentThread().getContextClassLoader)
- val cp = ois.readObject.asInstanceOf[Checkpoint]
- ois.close()
- fs.close()
+ var ois: ObjectInputStreamWithLoader = null
+ var cp: Checkpoint = null
+ Utils.tryWithSafeFinally {
+ val fis = fs.open(file)
+ // ObjectInputStream uses the last defined user-defined class loader in the stack
+ // to find classes, which maybe the wrong class loader. Hence, a inherited version
+ // of ObjectInputStream is used to explicitly use the current thread's default class
+ // loader to find and load classes. This is a well know Java issue and has popped up
+ // in other places (e.g., http://jira.codehaus.org/browse/GROOVY-1627)
+ val zis = compressionCodec.compressedInputStream(fis)
+ ois = new ObjectInputStreamWithLoader(zis,
+ Thread.currentThread().getContextClassLoader)
+ cp = ois.readObject.asInstanceOf[Checkpoint]
+ } {
+ ois.close()
+ fs.close()
--- End diff --
I know this code was there before, but, am I crazy -- are we not supposed to close HDFS `FileSystem` objects? this may be closing a cached `FileSystem`. It also looks like it meant to close `fis`. I suggest just closing `ois`.
No idea about the MiMa warning.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by tdas <gi...@git.apache.org>.
Github user tdas commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91147902
Jenkins, test this again.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90807277
[Test build #29833 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29833/consoleFull) for PR 5407 at commit [`5a98a86`](https://github.com/apache/spark/commit/5a98a869ca34a79798d14e45d9232a08381c8229).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90800714
[Test build #29831 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29831/consoleFull) for PR 5407 at commit [`6ce7d45`](https://github.com/apache/spark/commit/6ce7d45d36750c7176ad798f09711ec720949328).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90821110
[Test build #29833 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29833/consoleFull) for PR 5407 at commit [`5a98a86`](https://github.com/apache/spark/commit/5a98a869ca34a79798d14e45d9232a08381c8229).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91086071
[Test build #29914 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29914/consoleFull) for PR 5407 at commit [`a754adc`](https://github.com/apache/spark/commit/a754adc5429828beee6238bca21cd7d1520fc3b1).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-92158644
[Test build #30127 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30127/consoleFull) for PR 5407 at commit [`065999f`](https://github.com/apache/spark/commit/065999f4a09efca3df2eeb3efce3e4fbc2e25d04).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91417266
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29995/
Test PASSed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90831006
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29836/
Test PASSed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27943884
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -139,9 +139,11 @@ class CheckpointWriter(
// Write checkpoint to temp file
fs.delete(tempFile, true) // just in case it exists
val fos = fs.create(tempFile)
- fos.write(bytes)
- fos.close()
-
+ try{
+ fos.write(bytes)
+ }finally{
--- End diff --
The same as above.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r28156639
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -248,18 +253,22 @@ object CheckpointReader extends Logging {
checkpointFiles.foreach(file => {
logInfo("Attempting to load checkpoint from file " + file)
try {
- val fis = fs.open(file)
- // ObjectInputStream uses the last defined user-defined class loader in the stack
- // to find classes, which maybe the wrong class loader. Hence, a inherited version
- // of ObjectInputStream is used to explicitly use the current thread's default class
- // loader to find and load classes. This is a well know Java issue and has popped up
- // in other places (e.g., http://jira.codehaus.org/browse/GROOVY-1627)
- val zis = compressionCodec.compressedInputStream(fis)
- val ois = new ObjectInputStreamWithLoader(zis,
- Thread.currentThread().getContextClassLoader)
- val cp = ois.readObject.asInstanceOf[Checkpoint]
- ois.close()
- fs.close()
+ var ois: ObjectInputStreamWithLoader = null
+ var cp: Checkpoint = null
+ Utils.tryWithSafeFinally {
+ val fis = fs.open(file)
+ // ObjectInputStream uses the last defined user-defined class loader in the stack
+ // to find classes, which maybe the wrong class loader. Hence, a inherited version
+ // of ObjectInputStream is used to explicitly use the current thread's default class
+ // loader to find and load classes. This is a well know Java issue and has popped up
+ // in other places (e.g., http://jira.codehaus.org/browse/GROOVY-1627)
+ val zis = compressionCodec.compressedInputStream(fis)
+ ois = new ObjectInputStreamWithLoader(zis,
+ Thread.currentThread().getContextClassLoader)
+ cp = ois.readObject.asInstanceOf[Checkpoint]
+ } {
+ ois.close()
--- End diff --
Almost ready but this needs a guard against `null`. Otherwise LGTM.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/5407
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90854876
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29848/
Test PASSed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91400437
[Test build #29995 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29995/consoleFull) for PR 5407 at commit [`ef862d6`](https://github.com/apache/spark/commit/ef862d64ee129ffe0f74949147ec0c2576bbd5be).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90816329
[Test build #29836 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29836/consoleFull) for PR 5407 at commit [`c877da7`](https://github.com/apache/spark/commit/c877da72719ef022a978c83b061008d3044ff4a1).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27953289
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -187,9 +189,13 @@ class CheckpointWriter(
val bos = new ByteArrayOutputStream()
val zos = compressionCodec.compressedOutputStream(bos)
val oos = new ObjectOutputStream(zos)
- oos.writeObject(checkpoint)
- oos.close()
- bos.close()
+ try {
+ oos.writeObject(checkpoint)
+ } finally {
+ oos.close()
+ bos.close()
--- End diff --
Here's it's sufficient to close `oos` only. `bos.close()` is a no-op anyway, but chained writers close their delegates.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91148186
[Test build #643 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/643/consoleFull) for PR 5407 at commit [`a754adc`](https://github.com/apache/spark/commit/a754adc5429828beee6238bca21cd7d1520fc3b1).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90808478
[Test build #29831 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29831/consoleFull) for PR 5407 at commit [`6ce7d45`](https://github.com/apache/spark/commit/6ce7d45d36750c7176ad798f09711ec720949328).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90822661
[Test build #29834 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29834/consoleFull) for PR 5407 at commit [`44dca71`](https://github.com/apache/spark/commit/44dca7151bfc3c5e5edf098885938dd0562099a4).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90821116
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29833/
Test FAILed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27944086
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -187,16 +189,17 @@ class CheckpointWriter(
val bos = new ByteArrayOutputStream()
val zos = compressionCodec.compressedOutputStream(bos)
val oos = new ObjectOutputStream(zos)
- oos.writeObject(checkpoint)
- oos.close()
- bos.close()
try {
+ oos.writeObject(checkpoint)
executor.execute(new CheckpointWriteHandler(
checkpoint.checkpointTime, bos.toByteArray, clearCheckpointDataLater))
logDebug("Submitted checkpoint of time " + checkpoint.checkpointTime + " writer queue")
} catch {
case rej: RejectedExecutionException =>
logError("Could not submit checkpoint task to the thread pool executor", rej)
+ }finally{
--- End diff --
Also here. Style should keep the same.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90822668
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29834/
Test FAILed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90832887
[Test build #29848 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29848/consoleFull) for PR 5407 at commit [`824adb3`](https://github.com/apache/spark/commit/824adb3fceb50fc7d6bef054fcd9402ca82796f9).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91087668
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29914/
Test FAILed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27953257
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -139,9 +139,11 @@ class CheckpointWriter(
// Write checkpoint to temp file
fs.delete(tempFile, true) // just in case it exists
val fos = fs.create(tempFile)
- fos.write(bytes)
- fos.close()
-
+ try {
--- End diff --
Look at the new `Utils.tryWithSafeFinally` method which is used in a lot of the same contexts in the code now. It prevents an exception on closing a writer from masking the original exception.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91087655
[Test build #29914 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29914/consoleFull) for PR 5407 at commit [`a754adc`](https://github.com/apache/spark/commit/a754adc5429828beee6238bca21cd7d1520fc3b1).
* This patch **fails MiMa tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90830987
[Test build #29836 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29836/consoleFull) for PR 5407 at commit [`c877da7`](https://github.com/apache/spark/commit/c877da72719ef022a978c83b061008d3044ff4a1).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27953529
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -255,19 +263,24 @@ object CheckpointReader extends Logging {
// loader to find and load classes. This is a well know Java issue and has popped up
// in other places (e.g., http://jira.codehaus.org/browse/GROOVY-1627)
val zis = compressionCodec.compressedInputStream(fis)
- val ois = new ObjectInputStreamWithLoader(zis,
- Thread.currentThread().getContextClassLoader)
- val cp = ois.readObject.asInstanceOf[Checkpoint]
- ois.close()
- fs.close()
- cp.validate()
+ ois = new ObjectInputStreamWithLoader(zis,
+ Thread.currentThread().getContextClassLoader)
+ cpOption = Some(ois.readObject.asInstanceOf[Checkpoint])
logInfo("Checkpoint successfully loaded from file " + file)
- logInfo("Checkpoint was generated at time " + cp.checkpointTime)
- return Some(cp)
} catch {
case e: Exception =>
logWarning("Error reading checkpoint from file " + file, e)
+ } finally {
+ if (ois != null) {
+ ois.close()
+ }
+ fs.close()
+ }
+ cpOption.foreach { cp =>
+ cp.validate()
+ logInfo("Checkpoint was generated at time " + cp.checkpointTime)
}
+ return cpOption
--- End diff --
I think this changes the semantics. before, an exception should ultimately mean a `SparkException. Now, you return `None`. In fact isn't the final statement unreachable now since the `finally` block returns?
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-92177938
[Test build #30127 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/30127/consoleFull) for PR 5407 at commit [`065999f`](https://github.com/apache/spark/commit/065999f4a09efca3df2eeb3efce3e4fbc2e25d04).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by zhichao-li <gi...@git.apache.org>.
Github user zhichao-li commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27944362
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -247,6 +250,7 @@ object CheckpointReader extends Logging {
val compressionCodec = CompressionCodec.createCodec(conf)
checkpointFiles.foreach(file => {
logInfo("Attempting to load checkpoint from file " + file)
+ var ois:ObjectInputStreamWithLoader = null
--- End diff --
updated :)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/5407#discussion_r27943877
--- Diff: streaming/src/main/scala/org/apache/spark/streaming/Checkpoint.scala ---
@@ -139,9 +139,11 @@ class CheckpointWriter(
// Write checkpoint to temp file
fs.delete(tempFile, true) // just in case it exists
val fos = fs.create(tempFile)
- fos.write(bytes)
- fos.close()
-
+ try{
--- End diff --
White space after `try`.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90808482
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29831/
Test FAILed.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6762]Fix potential resource leaks in Ch...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-91182196
[Test build #643 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/643/consoleFull) for PR 5407 at commit [`a754adc`](https://github.com/apache/spark/commit/a754adc5429828beee6238bca21cd7d1520fc3b1).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Fix potential resource leaks in CheckPoint Che...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5407#issuecomment-90808630
[Test build #29834 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29834/consoleFull) for PR 5407 at commit [`44dca71`](https://github.com/apache/spark/commit/44dca7151bfc3c5e5edf098885938dd0562099a4).
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org