[improve][fn] Allow unknown fields in connectors config #20116
Conversation
...ctions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java
Outdated
Show resolved
Hide resolved
| if (configClass != null) { | ||
| final Object configInstance = Reflections.createInstance(configClass, | ||
| Thread.currentThread().getContextClassLoader()); | ||
| final List<String> allFields = |
There was a problem hiding this comment.
I am not sure that this is correct.
ObjectMapper should follow Java Beans conventions and use getter/setters together with public fields.
We should use some ObjectMapper utilities here in order to ensure that we are doing the right thing
There was a problem hiding this comment.
I've found a way to use the same deserializer used by jackson mapper, so this should cover all the cases when jackson mapper is used by the sink/source
|
|
||
| for (String s : config.keySet()) { | ||
| if (!allFields.contains(s)) { | ||
| log.warn("Field '{}' not defined in the {} configuration {}, the field will be ignored", |
There was a problem hiding this comment.
maybe this should be logged as "ERROR", WARNINGs tend to be ignored by alert systems
| + "Ignoring unknown fields makes possible to keep the new configuration and " | ||
| + "only rollback the connector." | ||
| ) | ||
| private boolean ignoreUnknownConfigFields = false; |
There was a problem hiding this comment.
This configuration applies to the instances of the functions/connectors, and not to the function worker (that already ignores unknown fields)
what about 'functionsIgnoreUnknownConfigFields' ? (maybe we can do better, but connectors are actually functions)
There was a problem hiding this comment.
I thought about that and I agree. But there are other properties that are meant to be used by the java runner like
maxPendingAsyncRequests, exposeAdminClientEnabled, additionalJavaRuntimeArguments so I'd prefer to stick with this convention
(cherry picked from commit f7c0b3c)
Motivation
Most of the java connectors (sinks and sources) map the configuration to a java class. Pulsar exposes a method to convert the configuration (
IOConfigUtils#loadWithSecrets) using a jackson mapper. However is up to the connector to decide how to convert the configuration.Most of the builtin java connectors do not allow unknown fields in their configuration. This is usually a good thing since it alerts the user that something is malconfigured. But this doesn't leave space for connectors' rollbacks.
The scenario which is not possible to cover is:
Modifications
ignoreUnknownConfigFields(default false) that strips out fields not supported in the current sink/source config. The sink/source config class is retrieved from the service definition (sinkConfigClassandsourceConfigClass)Verifying this change
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository