Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore RuntimeIOException for use #5640

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

danielcweeks
Copy link
Contributor

I'd like to open this proposal to restore RuntimeIOException for continued use and remove the deprecation.

This was originally marked deprecated when java introduced the UncheckedIOException which initially seemed like a more standard replacement, but there are a few issues.

  1. UncheckedIOException requires a cause, which is not present in a number of usages of RuntimeIOException. This complicates the code or requires us to change the exception handling. For example:
      if (!file.getParentFile().isDirectory() && !file.getParentFile().mkdirs()) {
        throw new RuntimeIOException(
            "Failed to create the file's directory at %s.", file.getParentFile().getAbsolutePath());
      }

becomes

      if (!file.getParentFile().isDirectory() && !file.getParentFile().mkdirs()) {
        throw new UncheckedIOException(new IOException(
            String.format("Failed to create the file's directory at %s.", file.getParentFile().getAbsolutePath())));
      }
  1. RuntimeIOException also allows for string formatting which is used extensively in creating messages (also seen in the example above.
  2. This class is used pervasively, so there there's a lot of change to deprecate, but RuntimeIOException already extends UncheckedIOException, so there is very little practical improvement.

Alternatives:

Another option for deprecation would be to remove the constructors for RuntimeIOException that take a cause and update any existing usages that actually have an IO cause to use UncheckedIOException. This would leave `RuntimeIOException as a thin wrapper for cases where we want to throw IOException without an originating cause to keep those usages clean

@rdblue
Copy link
Contributor

rdblue commented Aug 26, 2022

I think this is a reasonable way to handle the issue, although I would like to be consistent with the exceptions that are thrown from Iceberg. If we keep RuntimeIOException, then we should throw it everywhere rather than throwing UncheckedIOException.

@ajantha-bhat
Copy link
Member

ajantha-bhat commented Sep 7, 2022

Just linking the opposite work PR here
#4776

[125 places] Replace deprecated RuntimeIOException with UncheckedIOException if the cause is having IOException already.
[10 places] Introduce RuntimeUncheckedIOException if the cause is not having any IOException for RuntimeIOException.

@RussellSpitzer
Copy link
Member

I have no problems with keeping RuntimeIOException for all the cases where we don't have an underlying cause. Let's just try to be careful in the future that we don't actually swallow causes which I think as one of our original impetuses for not allowing RuntimeIOExceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants