hkwi commented on code in PR #17329:
URL: https://github.com/apache/iceberg/pull/17329#discussion_r3636566464
##########
api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java:
##########
@@ -159,14 +159,14 @@ public Map<String, String> properties() {
@Override
public void close() {
- io.close();
-
if (em instanceof Closeable) {
Review Comment:
Good point — I agree that keeping `io.close()` in one place would make this
cleaner. How about using a nullable try-with-resources resource instead?
```java
@Override
public void close() {
try (Closeable closeableManager =
em instanceof Closeable ? (Closeable) em : null) {
io.close();
} catch (IOException e) {
throw new UncheckedIOException("Failed to close encryption manager", e);
}
}
```
This removes the duplication while preserving try-with-resources suppression
semantics. If both `io.close()` and `closeableManager.close()` fail, the FileIO
failure remains the primary exception and the manager failure is added as
suppressed. With the proposed `finally`, the `UncheckedIOException` from the
manager would replace the FileIO failure. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
