[GitHub] [flink] bowenli86 commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception
bowenli86 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-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. I'm not sure if catching the unchecked exception here is optimal for users as they are not declared in API signature and users aren't sure what unchecked exceptions will be thrown unless looking at implementations. Besides, loading/unloading module will most likely happen via config or interactive programming, in which case the error message matters more. If users ran into this via a table API program, that's a bug and they should fix their program rather than catching the exceptions. Thus I think throwing ValidationException is fine, and it aligns better with other parts of Flink SQL. Though it seems a bit unnecessary given the above use case explained, I also don't have strong opinions of 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] bowenli86 commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception
bowenli86 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-548941834 @dawidwys @lirui-apache @zjuwangg 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] bowenli86 commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception
bowenli86 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-548600306 @dawidwys @xuefuz @lirui-apache @zjuwangg 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] bowenli86 commented on issue #10053: [FLINK-14578][table] load/unloadModule() should throw RuntimeException rather than checked exception
bowenli86 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-548152941 @dawidwys @xuefuz @lirui-apache @zjuwangg 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: us...@infra.apache.org With regards, Apache Git Services
