gharris1727 commented on code in PR #14309:
URL: https://github.com/apache/kafka/pull/14309#discussion_r1316465707
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java:
##########
@@ -380,6 +387,110 @@ protected Map<String, ConfigValue>
validateSourceConnectorConfig(SourceConnector
return configDef.validateAll(config);
}
+ private <T> ConfigInfos validateConverterConfig(
+ Map<String, String> connectorConfig,
+ ConfigValue converterConfigValue,
+ Class<T> converterInterface,
+ Function<T, ConfigDef> configDefAccessor,
+ String converterName,
+ String converterProperty,
+ ConverterType converterType
+ ) {
+ String converterClass = connectorConfig.get(converterProperty);
+
+ if (converterClass == null
+ || converterConfigValue == null
+ || !converterConfigValue.errorMessages().isEmpty()
+ ) {
+ // Either no custom converter was specified, or one was specified
but there's a problem with it.
+ // No need to proceed any further.
+ return null;
+ }
+
+ T converterInstance;
+ try {
+ converterInstance = Utils.newInstance(converterClass,
converterInterface);
+ } catch (ClassNotFoundException | RuntimeException e) {
+ log.error("Failed to instantiate {} class {}; this should have
been caught by prior validation logic", converterName, converterClass, e);
+ converterConfigValue.addErrorMessage("Failed to load class " +
converterClass + (e.getMessage() != null ? ": " + e.getMessage() : ""));
+ return null;
+ }
+
+ try (Utils.UncheckedCloseable close = () ->
Utils.maybeCloseQuietly(converterInstance, converterName + " " +
converterClass);) {
Review Comment:
> It felt slightly more readable to use a try-with-resources block than a
finally block (especially since we don't have any catch blocks).
I suppose there's some subjectivity involved here, since I found the
UncheckedClosable and explicit lambda to be a bit hard to read initially, but
understood after some inspection. Using try-finally without any catch clauses
is a pretty normal arrangement, and I think more developers would be used to it
as compared to using a lambda along with our special UncheckedClosable.
AFAIU the try-with-resources construct was added to help with cleaning up
AutoClosable resources which can throw exceptions both during opening and
closing, where it becomes tedious to set up the `finally` to perform the
cleanup correctly. In this specific situation, the `newInstance` (open) errors
are handled by a separate `try`, and the close errors are handled by
`closeQuietly`, so none of the value-add of the try-with-resources is apparent.
I See what you mean though, as we _do_ have exceptions from open and close,
and we have somewhat tedious error handling surrounding them. But since the
objects we're instantiating are "sometimes AutoClosable", the
try-with-resources type checking is going to get in the way.
Using try-with-resources to handle open and close together, you could have a
wrapper class `MaybeClosable<T> implements UncheckedClosable, Supplier<T>`
along with a method `static <T> MaybeClosable<T> quiet(T, String)` that you
would call like this:
```
try (MaybeClosable<Converter> wrapper =
MaybeClosable.quiet(Utils.newInstance(...), "converter (for validation)") {
Converter converter = wrapper.get();
// do stuff
} catch (RuntimeException e) {
// exceptions from newInstance and do stuff
}
// exceptions from close are logged instead of propagated/suppressed
```
I think that would type-check but I haven't tried it out myself. Everything
is just a rough suggestion, so please iterate on the names or ergonomics if you
like the idea.
Without the closeQuietly semantics, it would look like this:
```
try (MaybeClosable<Converter> wrapper =
MaybeClosable.propagate(Utils.newInstance(...)) {
Converter converter = wrapper.get();
// do stuff
} catch (RuntimeException e) {
// exceptions from newInstance and do stuff
}
// exceptions from close are not checked, but propagated or suppressed.
```
> Is this correct? The interface was introduced in
https://github.com/apache/kafka/pull/8618, where it was used on the left-hand
side of a try-with-resources assignment that captured a method reference which
did not throw a checked exception.
When I said "exception throwing operation" I didn't mean "method that throws
a checked exception", because I was thinking about how methods can throw
RuntimeExceptions whether or not they have checked exceptions in the signature.
I probably should have said "method that throws unchecked exceptions" to be
unambiguous. Yes this PR and the linked PR both did not have checked
exceptions, but they differ because one throws RuntimeExceptions and one does
not.
In that PR I used try-with-resources because I wanted the
propagate-or-suppress logic built into the try-with-resources for free, instead
of the shadowing behavior that a normal finally clause has. If I just wanted to
log exceptions in the finally operation, I think I would have used a
`Utils.closeQuietly` call in the finally instead. Recently in #14277 i did a
bare log in the finally instead of try-with-resources, as I didn't need the
propagate-or-suppress logic. Maybe I should have used closeQuietly, but I
didn't think of that at the time for whatever reason.
Most of my friction with the as-implemented version is that you're using the
UncheckedClosable and all of the readability costs, without getting the benefit
of the propagate-or-suppress logic.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
