rdblue commented on a change in pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#discussion_r448535389
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -277,7 +277,7 @@ private static void deleteFiles(FileIO io,
Set<ManifestFile> allManifests) {
}
}
} catch (IOException e) {
- throw new RuntimeIOException(e, "Failed to read manifest file: " +
manifest.path());
+ throw new UncheckedIOException("Failed to read manifest file: " +
manifest.path(), e);
Review comment:
Yes, I think we would just make `RuntimeIOException` extend
`UncheckedIOException` and leave the current cases as they are, since there
isn't a lot of value to removing the class if it extends the more standard one.
I hear your concern about the formatting magic and order of arguments.
Here's the context:
1. We want to make it easy to add context into exception messages. It is too
common that an exception tells you what happened, but not where it happened. So
you might have a `NullPointerException` when writing a record field and not be
told which field you need to go look at. The idea behind adding `String.format`
into the constructors is to make it as easy as possible to build good exception
messages.
2. Because we accept a format string and `Object...` for message formatting,
we can't add the wrapped exception at the end of the arg list. The varargs
parameter always consumes it. So we had to move it to the beginning.
I though this was a reasonable trade-off for better exception messages. Not
sure if other people agree, though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
