PARQUET-1922: Deprecate IOExceptionUtils #825
Conversation
|
|
||
| public void close() { | ||
| IOExceptionUtils.closeQuietly(out); | ||
| public void close() throws IOException { |
There was a problem hiding this comment.
The inherited close() method throws IOException so this is fine to do
| @Override | ||
| public void close() { | ||
| arrayOut.close(); | ||
| out.close(); |
There was a problem hiding this comment.
out in this case is wraps arrayOut, and it has no buffering, so it does not need to be explicitly closed, or better, in the future, out should be closed only and arrayOut will be closed implicitly. Anyway, I came to this conclusion because this is how close() is implemented in a couple of other Writer classes as well.
There was a problem hiding this comment.
I would either close only arrayOut but with comments about what you have written here or close only out and catching the IOException that would never occur and comment in the catch block similarly.
There was a problem hiding this comment.
@gszadovszky Thanks for the review!
I think this line of work should be pursued in a different ticket. This change makes this PR's changes possible, but also puts it in line with other implementations:
So if we want to change how things are closed (nice, but technically not required) it can be addressed holistically.
There was a problem hiding this comment.
I am fine fixing this in another ticket. What I think is a key in this change is to put the related comments (out.close is not required because...) in the code as well as in this review.
There was a problem hiding this comment.
@gszadovszky OK, finally got back to this. Ha. I added a comment. Please consider for merge.
| @Override | ||
| public void close() { | ||
| arrayOut.close(); | ||
| out.close(); |
There was a problem hiding this comment.
I would either close only arrayOut but with comments about what you have written here or close only out and catching the IOException that would never occur and comment in the catch block similarly.
Yes. That is true, but by design. |
|
Doing a little too much here. Just going to swallow exceptions and keep the previous behavior. I can change API in another ticket. |
* 'master' of https://github.com/apache/parquet-mr: (222 commits) PARQUET-2052: Integer overflow when writing huge binary using dictionary encoding (apache#910) PARQUET-2041: Add zstd to `parquet.compression` description of ParquetOutputFormat Javadoc (apache#899) PARQUET-2050: Expose repetition & definition level from ColumnIO (apache#908) PARQUET-1761: Lower Logging Level in ParquetOutputFormat (apache#745) PARQUET-2046: Upgrade Apache POM to 23 (apache#904) PARQUET-2048: Deprecate BaseRecordReader (apache#906) PARQUET-1922: Deprecate IOExceptionUtils (apache#825) PARQUET-2037: Write INT96 with parquet-avro (apache#901) PARQUET-2044: Enable ZSTD buffer pool by default (apache#903) PARQUET-2038: Upgrade Jackson version used in parquet encryption. (apache#898) Revert "[WIP] Refactor GroupReadSupport to unuse deprecated api (apache#894)" PARQUET-2027: Fix calculating directory offset for merge (apache#896) [WIP] Refactor GroupReadSupport to unuse deprecated api (apache#894) PARQUET-2030: Expose page size row check configurations to ParquetWriter.Builder (apache#895) PARQUET-2031: Upgrade to parquet-format 2.9.0 (apache#897) PARQUET-1448: Review of ParquetFileReader (apache#892) PARQUET-2020: Remove deprecated modules (apache#888) PARQUET-2025: Update Snappy version to 1.1.8.3 (apache#893) PARQUET-2022: ZstdDecompressorStream should close `zstdInputStream` (apache#889) PARQUET-1982: Random access to row groups in ParquetFileReader (apache#871) ... # Conflicts: # parquet-column/src/main/java/org/apache/parquet/example/data/simple/SimpleGroup.java # parquet-hadoop/pom.xml # parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java
2 participants
Make sure you have checked all steps below.
Jira
Tests
Commits
Documentation