-
Notifications
You must be signed in to change notification settings - Fork 29k
SPARK-16420: Ensure compression streams are closed. #14093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -349,12 +349,19 @@ private long[] mergeSpillsWithFileStream( | |
| for (int i = 0; i < spills.length; i++) { | ||
| final long partitionLengthInSpill = spills[i].partitionLengths[partition]; | ||
| if (partitionLengthInSpill > 0) { | ||
| InputStream partitionInputStream = | ||
| new LimitedInputStream(spillInputStreams[i], partitionLengthInSpill); | ||
| if (compressionCodec != null) { | ||
| partitionInputStream = compressionCodec.compressedInputStream(partitionInputStream); | ||
| InputStream partitionInputStream = null; | ||
| boolean innerThrewException = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a little bit strange - can we change it to do try catch and at the end of try we close with the flag false, and in catch we close with the flag true and then eliminate the variable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the typical way to use I'm reluctant to change from the standard pattern for aesthetic reasons when
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree with @rdblue here; note that we use this |
||
| try { | ||
| partitionInputStream = | ||
| new LimitedInputStream(spillInputStreams[i], partitionLengthInSpill, false); | ||
| if (compressionCodec != null) { | ||
| partitionInputStream = compressionCodec.compressedInputStream(partitionInputStream); | ||
| } | ||
| ByteStreams.copy(partitionInputStream, mergedFileOutputStream); | ||
| innerThrewException = false; | ||
| } finally { | ||
| Closeables.close(partitionInputStream, innerThrewException); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that it matters much, but what about also using the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the usual equivalent for Java code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh duh it's Java. I have made that mistake about 10 times now |
||
| } | ||
| ByteStreams.copy(partitionInputStream, mergedFileOutputStream); | ||
| } | ||
| } | ||
| mergedFileOutputStream.flush(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a blank line here?