bowenli86 edited a comment on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception URL: https://github.com/apache/flink/pull/10053#issuecomment-548984963 > Thanks for the contribution. The diff itself looks fine. However, wouldn't it be better if we change existing exception classes (such as ModuleNotFoundException) to runtime? > > The downside of having a general exception (such as ValidationException) is the difficult for an exception handler to differentiate the root cause. (Otherwise, one would have to parse the exception msg.) > > Thus, I prefer retaining the existing exception classes but changing them to runtime. maybe catching unchecked exception isn't optimal for users as they are not declared in API and users aren't sure what unchecked exceptions will be thrown unless looking at impls. Loading/unloading module will most likely happen via config or interactive programming, where error messages matter more. If users ran into this in a program, their program would have bugs and they'd better fix it rather than catching the exceptions. Thus throwing ValidationException is fine, and it aligns better with other parts of Flink SQL. I'm also fine with making ModuleNotExistException/ModuleAlreadyExistException RuntimeException or extend ValidationException.
---------------------------------------------------------------- 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] With regards, Apache Git Services
