[GitHub] [flink] dawidwys commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception

[GitBox <gi...@apache.org>]

dawidwys commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception
URL: https://github.com/apache/flink/pull/10053#issuecomment-549296660
 
 
   Hi, as I said before I am not entirely against having a custom exception as long as they are `RuntimeExceptions`. There are a few very important things to consider when adding one.
   
   1. We do want to differentiate between user problem (ValidationException) and system problem (TableException). For this to work we should make sure all exceptions that we throw extend from one or the other. IMHO, this is very helpful e.g. in case of SQL client, where we could close the client on `TableException` as this is an unrecoverable problem, whereas on `ValidationException` we could just print a message to the user.
   2. If we want to open up the exceptions part we should think through the class hierarchy well. It would probably make sense to have a class in between the `ValidationException` and `ModuleAlreadyExistException` that tells that it comes from the `Module` module. The problem then is how do we combine the `ModuleException` with `TableException` and `ValidationException`, as there might be exceptions in both of those categories. Another option would be to extend the ValidationException to add additional context to it or to other exceptions.
   3. It is easier to introduce a new more specific exception in the future than to remove it. If we want to remove some specific exception it requires changes to the client code. This is why I am a bit hesitant towards opening the discussion on exceptions hierarchy, as this is not such an easy thing as I tried to describe in the 2. point.
   4. I see no immediate benefit of throwing a `ModuleAlreadyExistException` instead of a `ValidationException` in these methods. It adds a contract that we would have to adhere to in the future, as I described in the 3. point.
   5. This last point does not necessarily relates to the changes in this PR, but wanted to mention that anyway. We should be careful to not end up with a control flow based on `Exception`s as this is much harder to program against and is less performant than reacting to the return value. I do fear that having dozens of exceptions encourages it.
   
   Just to sum up. I am ok with having custom **unchecked** exceptions in these methods. I would really appreciate though if the class hierarchy of these is improved.

----------------------------------------------------------------
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


With regards,
Apache Git Services