XComp commented on code in PR #22072:
URL: https://github.com/apache/flink/pull/22072#discussion_r1123179109
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/ServiceLoaderUtil.java:
##########
@@ -44,6 +51,20 @@ static <T> List<LoadResult<T>> load(Class<T> clazz,
ClassLoader classLoader) {
} catch (NoSuchElementException e) {
break;
} catch (Throwable t) {
+ if (t instanceof NoClassDefFoundError) {
+ LOG.debug(
+ "NoClassDefFoundError when loading a "
+ + clazz.getCanonicalName()
+ + ". This is expected when trying to load
a format dependency but no flink-connector-files is loaded.",
+ t);
+ // After logging, we just ignore this failure
+ continue;
+ }
+
+ if (!clazz.isInstance(t)) {
Review Comment:
imho, this if condition still doesn't make sense. And the error handling
should probably live in the calling method `FactoryUtil.discoverFactories`.
`ServiceLoaderUtil.load` seems to be more general. I provided a commit to show
what I had in mind:
[42453004](https://github.com/fsk119/flink/pull/1/commits/42453004898473a6e28b78f7603b30c3b1e24951)
in [PR fsk119/flink:#1](https://github.com/fsk119/flink/pull/1/). But we could
even go one step further and delete `ServiceLoaderUtil` completely. imho, it
doesn't add much value to the codebase as it's only called in
`FactoryUtil.discoverFactories`.
But see [PR fsk119/flink:#1](https://github.com/fsk119/flink/pull/1/) more
like a suggestion. ...it's your call.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
