[FLINK-5854] [core] Add base Flink Exception classes #3368
Conversation
| * Class.forName(classname).asSubclass(TheType.class).newInstance(); | ||
| * } | ||
| * catch (ClassNotFoundException | ClassCastException | InstantiationException | IllegalAccessException e) { | ||
| * throw new DynamicCodeLoadingException(e); |
There was a problem hiding this comment.
there is no constructor that matches this line of the javadoc.
There was a problem hiding this comment.
Agreed, will fix this
| * @param message The exception message | ||
| * @param cause The exception that caused this exception | ||
| */ | ||
| public DynamicCodeLoadingException(String message, Throwable cause) { |
There was a problem hiding this comment.
Would it make sense to make this constructor more explicit in the type of exceptions it accepts? (i.e. one constructor each for the exceptions that are typically thrown in situations that we want to cover)
It's probably just bloat, but maybe it would prevent misuse of this exception.
There was a problem hiding this comment.
There may be other cases for this exception that we do not anticipate, like for shipping of generated code in bytes that is dynamically converted to classes (not sure what exceptions that will entail, but probably ClassFormatError and so on).
| /** | ||
| * Creates a new exception with a null message and null cause. | ||
| */ | ||
| public FlinkRuntimeException() { |
There was a problem hiding this comment.
Is there a reasonable use-case for an exception without an error message or cause?
|
Could you name 1 or 2 examples for situations where you think it is appropriate to throw a |
|
@zentol There are many places in the runtime that declare The only place where |
|
It is arguable whether exceptions should ever have a constructor without a message, I simply did that here for convenience. I have no strong feelings about removing the zero argument constructors. |
|
Any reservations against merging this after addressing the comments above? |
|
This looks good, but what is the reason behind the empty constructors that create an exception without message or cause? |
|
Hey Stephan! The changes look very good. Thanks also for your explanations, I think this is something that we should focus on more during code reviews. Actually, a section in the Internals or Contribution docs or Wiki would be helpful. A question by example: |
|
Big +1 from my side. I think that more specific exceptions helps to make people think more about the cause of an exception and, thus, also how it should be handled. Especially the fact that we always catch @uce I think we should not per se wrap all current |
|
Thanks for the comments. Will address the issues and remove the "no message, no cause" constructors. We should not encourage exceptions without information. |
6 participants
This pull request adds two exception base classes:
FlinkExceptionandFlinkRuntimeException.They are useful in improving the way certain parts of the code handle exceptions.
FlinkExceptionis a base class for checked exceptions that indicate that something related to using Flink went wrong. It is helpful, because letting a method throwFlinkExceptionrather thanExceptionalready helps to not include all of Java's runtime exceptions, which indicate programming errors, rather than situations that should be recovered.FlinkRuntimeExceptionas a Flink-specific subclass ofRuntimeExceptioncomes in handy in places where no exceptions were declared, for example when reusing an interface that does not declare exceptions.Important: This does not mean we should just declare
FlinkExceptioneverywhere and throw and catchFlinkExceptionandFlinkRuntimeExceptionarbitrarily. Exception handling remains a careful and conscious task.This also adds the
DynamicCodeLoadingExceptionsubclass ofFlinkExceptionas an example.