You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/07/01 15:44:54 UTC
[GitHub] [iceberg] massdosage opened a new pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
massdosage opened a new pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154
Following on from the discussion at https://github.com/apache/iceberg/pull/1103/files#r447344934 - this PR replaces Iceberg's `RuntimeIOException` with the more standard `UncheckedIOException` available from Java 8.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue edited a comment on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-653612452
Looks good to me. Thanks for fixing this, @massdosage! I'll merge this when tests pass.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] massdosage commented on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
massdosage commented on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-652947677
What do you think of marking `RuntimeIOException` as "deprecated" to encourage the usage of `UncheckedIOException` instead? Let me know, otherwise I can remove that too and just leave the `extends` part in.
I've reverted everything in `api` to use `RuntimeIOException` but if you think its usages in other subprojects could also cause issues I can revert it everywhere (other than possibly a few tests which seemed to be using it but really don't need to).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-653620272
Tests are passing, but the status sync to GitHub is flaky. I'll merge this.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] massdosage commented on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
massdosage commented on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-653559287
OK, I reverted all the usages except:
* a few tests where I think it's alright to move over to `UncheckedIOException` since they aren't part of the API
* the `mr` package since this is new and I think among us we're OK with dealing with this change
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#discussion_r448478687
##########
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:
+1 for this idea. That would be fewer changes and is a binary compatible change.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdsr commented on a change in pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdsr commented on a change in pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#discussion_r448475846
##########
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:
I do see some public APIs are being affected. E.g `createTable`. Do you guys think it is safer to make ` RuntimeOIException` extend `UncheckedIOE` instead of replacing RuntimeIOException completely?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] massdosage commented on a change in pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
massdosage commented on a change in pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#discussion_r448488064
##########
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:
So would we just make the class extend `UncheckedIOException` and leave everything throwing it as it is now? I personally don't like the way in the arguments it puts the exception first and the message second, I find this confusing as most other Java Exceptions do it the other way around. There is also the "magic" it performs to `String.format` things which again was a bit of a surprise and forced me to look into the internals which I rarely have to do with other Exception types I've used.
You guys know a lot more about how this is used but since its an Unchecked Exception is API compatibility such an issue? Perhaps there is a defined set of classes where it needs to be kept but can be removed everywhere else?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-653141238
+1 to marking `RuntimeIOException` as deprecated and nudging the user to catch `UncheckedIOException`.
I think we need to revert all of the conversions, not just the ones in API. Because we're using unchecked exceptions, many of the other places this is thrown will bubble up through the API.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#issuecomment-653612452
Looks good to me, thanks for fixing this, @massdosage! I'll merge this when tests pass.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] massdosage commented on a change in pull request #1154: Replaced RuntimeIOException with Java 8 UncheckedIOException
Posted by GitBox <gi...@apache.org>.
massdosage commented on a change in pull request #1154:
URL: https://github.com/apache/iceberg/pull/1154#discussion_r448920761
##########
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:
OK, the above context makes sense, but I still think the principle of "least surprises" would lean me to not do this and just make sure people write good messages when they create exceptions. Anyway, at some point I think it would be good to to have a DEVELOPERS.md (or something equivalent) for getting new developers up to speed with things like the above where the project may differ from what they are used to.
I'll update this PR for `RuntimeIOException` to extend `UncheckedIOException`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org