MINOR: Close already-opened input readers when ParquetRewriter setup fails#3662
Open
anxkhn wants to merge 1 commit into
Open
MINOR: Close already-opened input readers when ParquetRewriter setup fails#3662anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
…fails ParquetRewriter.getFileReaders opens a TransParquetFileReader for each input file into a local list. If opening a later file throws IOException, it was rethrown as IllegalArgumentException without closing the readers already opened for the earlier files, leaking their SeekableInputStream handles (ParquetRewriter.close() only ends the writer and does not close input readers). One corrupt, missing, or permission-denied file among several valid ones could therefore leak up to N open input streams per failed rewrite/merge/join setup. Close the readers already opened before rethrowing, using AutoCloseables.close so every reader is released and any close failures are aggregated as suppressed exceptions on the original IllegalArgumentException. Add a ParquetRewriterTest case that supplies a valid input file followed by one that fails to open and asserts the first reader's stream is closed. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Member
|
I would recommend creating an issue for this. Or at least do not mark it as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
ParquetRewriter.getFileReadersopens aTransParquetFileReaderfor each inputfile into a local list. If opening a later file throws
IOException, it isrethrown as
IllegalArgumentExceptionwithout closing the readers already openedfor the earlier files, so their
SeekableInputStreamhandles leak.ParquetRewriter.close()only ends the writer and does not close the inputreaders, so nothing else releases them either. A single corrupt, missing, or
permission-denied file among several valid inputs therefore leaks up to N open
input streams per failed rewrite/merge/join setup.
What changes are included in this PR?
In
getFileReaders, close the readers already opened before rethrowing, usingorg.apache.parquet.util.AutoCloseables.close(...)so every reader is releasedand any close failures are aggregated as suppressed exceptions on the original
IllegalArgumentException. This matches the existing cleanup idiom in the module.The change is limited to the
catch (IOException)block; the success path isunchanged.
Are these changes tested?
Yes. A new
ParquetRewriterTestcase(
testInputFileReadersClosedWhenLaterInputFileFailsToOpen) supplies a validinput file (wrapped so it records whether the stream it hands out is closed)
followed by an
InputFilewhosenewStream()throws, asserts that constructingthe
ParquetRewriterthrowsIllegalArgumentException, and asserts the firstreader's stream was closed. It fails on the pre-fix code (the first stream is
left open) and passes with the fix. The full
ParquetRewriterTestclass passes(
Tests run: 96, Failures: 0, Errors: 0).Are there any user-facing changes?
No public API changes. The only behavioral difference is that a failed
ParquetRewritersetup no longer leaks the input streams it had already opened.