Skip to content

[SPARK-7490] [CORE] [Minor] MapOutputTracker.deserializeMapStatuses: close input streams#5982

Closed
evanj wants to merge 1 commit into
apache:masterfrom
evanj:master
Closed

[SPARK-7490] [CORE] [Minor] MapOutputTracker.deserializeMapStatuses: close input streams#5982
evanj wants to merge 1 commit into
apache:masterfrom
evanj:master

Conversation

@evanj

@evanj evanj commented May 7, 2015

Copy link
Copy Markdown

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

@stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.
@AmplabJenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

@evanj

evanj commented May 7, 2015

Copy link
Copy Markdown
Author

Actually, I'm not 100% sure it did pass, the output is somewhat confusing the last few lines are:

[info] ScalaTest
[info] Run completed in 1 hour, 15 minutes, 52 seconds.
[info] Total number of tests run: 647
[info] Suites: completed 33, aborted 0
[info] Tests: succeeded 647, failed 0, canceled 0, ignored 19, pending 0
[info] All tests passed.
[info] Passed: Total 650, Failed 0, Errors 0, Passed 650, Ignored 19
[error] (streaming/test:test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 4890 s, completed May 7, 2015 1:05:05 PM
[error] Got a return code of 1 on line 229 of the run-tests script.

@JoshRosen

Copy link
Copy Markdown
Contributor

Actually, do you mind creating a JIRA for this? That would help us to track backports, etc.

@JoshRosen

Copy link
Copy Markdown
Contributor

Jenkins, this is ok to test.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@SparkQA

SparkQA commented May 7, 2015

Copy link
Copy Markdown

Test build #32126 has started for PR 5982 at commit 0d76e85.

@SparkQA

SparkQA commented May 7, 2015

Copy link
Copy Markdown

Test build #32126 has finished for PR 5982 at commit 0d76e85.

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

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. Test FAILed.

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32126/
Test FAILed.

@andrewor14

Copy link
Copy Markdown
Contributor

retest this please. There's a likely possibility that the test failure is not related.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@SparkQA

SparkQA commented May 7, 2015

Copy link
Copy Markdown

Test build #32141 has started for PR 5982 at commit 0d76e85.

@andrewor14

Copy link
Copy Markdown
Contributor

Also @evanj would you mind at least putting [Minor] in the title? Otherwise people who did not read your PR description may ask you to open a JIRA.

@SparkQA

SparkQA commented May 7, 2015

Copy link
Copy Markdown

Test build #32141 has finished for PR 5982 at commit 0d76e85.

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

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. Test PASSed.

@AmplabJenkins

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32141/
Test PASSed.

@stephenh

stephenh commented May 7, 2015

Copy link
Copy Markdown

Nice catch.

@evanj evanj changed the title [CORE] MapOutputTracker.deserializeMapStatuses: close input streams [CORE] [Minor] MapOutputTracker.deserializeMapStatuses: close input streams May 8, 2015
@evanj

evanj commented May 8, 2015

Copy link
Copy Markdown
Author

I have some conflicting advice here; I guess I'll create a JIRA for it?

Re: nice catch: I hit a production bug (native memory leak) related to unclosed GZIP streams at Twitter, so I audited all the places that use this using our internal code search tool; This is one that came up. I think FindBugs would also likely find this.

@evanj evanj changed the title [CORE] [Minor] MapOutputTracker.deserializeMapStatuses: close input streams [SPARK-7490] [CORE] [Minor] MapOutputTracker.deserializeMapStatuses: close input streams May 8, 2015
asfgit pushed a commit that referenced this pull request May 8, 2015
…close input streams

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

Author: Evan Jones <[email protected]>

Closes #5982 from evanj/master and squashes the following commits:

0d76e85 [Evan Jones] [CORE] MapOutputTracker.deserializeMapStatuses: close input streams

(cherry picked from commit 25889d8)
Signed-off-by: Sean Owen <[email protected]>
asfgit pushed a commit that referenced this pull request May 8, 2015
…close input streams

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

Author: Evan Jones <[email protected]>

Closes #5982 from evanj/master and squashes the following commits:

0d76e85 [Evan Jones] [CORE] MapOutputTracker.deserializeMapStatuses: close input streams

(cherry picked from commit 25889d8)
Signed-off-by: Sean Owen <[email protected]>
@asfgit asfgit closed this in 25889d8 May 8, 2015
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
…close input streams

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

Author: Evan Jones <[email protected]>

Closes apache#5982 from evanj/master and squashes the following commits:

0d76e85 [Evan Jones] [CORE] MapOutputTracker.deserializeMapStatuses: close input streams
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
…close input streams

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

Author: Evan Jones <[email protected]>

Closes apache#5982 from evanj/master and squashes the following commits:

0d76e85 [Evan Jones] [CORE] MapOutputTracker.deserializeMapStatuses: close input streams
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…close input streams

GZIPInputStream allocates native memory that is not freed until close() or
when the finalizer runs. It is best to close() these streams explicitly.

stephenh made the same change for serializeMapStatuses in commit b0d884f. This is the same change for deserialize.

(I ran the unit test suite! it seems to have passed. I did not make a JIRA since this seems "trivial", and the guidelines suggest it is not required for trivial changes)

Author: Evan Jones <[email protected]>

Closes apache#5982 from evanj/master and squashes the following commits:

0d76e85 [Evan Jones] [CORE] MapOutputTracker.deserializeMapStatuses: close input streams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants