Hi Gunnar,
I think this KIP would be a great addition to Kafka Connect but it
looks like it's been abandoned.
Are you still interested in working on this? If you need some time or
help, that's fine, just let us know.
If not, no worries, I'm happy to pick it up if needed.
Thanks,
Mickael
On Wed, Dec 22, 2021 at 11:21 AM Tom Bentley <tbent...@redhat.com> wrote:
>
> Hi Gunnar,
>
> Thanks for the KIP, especially the careful reasoning about compatibility. I
> think this would be a useful improvement. I have a few observations, which
> are all about how we effectively communicate the contract to implementers:
>
> 1. I think it would be good for the Javadoc to give a bit more of a hint
> about what the validate(Map) method is supposed to do: At least call
> ConfigDef.validate(Map) with the provided configs (for implementers that
> can be achieved via super.validate()), and optionally apply extra
> validation for constraints that ConfigDef (and ConfigDef.Validator) cannot
> check. I think typically that would be where there's a dependency between
> two config parameters, e.g. if 'foo' is present that 'bar' must be too, or
> 'baz' and 'qux' cannot have the same value.
> 2. Can the Javadoc give a bit more detail about the return value of these
> new methods? I'm not sure that the implementer of a Transformation would
> necessarily know how the Config returned from validate(Map) might be
> "updated", or that updating ConfigValue's errorMessages is the right way to
> report config-specific errors. The KIP should be clear on how we expect
> implementers to report errors due to dependencies between multiple config
> parameters (must they be tied to a config parameter, or should the method
> throw, for example?). I think this is a bit awkward, actually, since the
> ConfigInfo structure used for the JSON REST response doesn't seem to have a
> nice way to represent errors which are not associated with a config
> parameter.
> 3. It might also be worth calling out that the expectation is that a
> successful return from the new validate() method should imply that
> configure(Map) will succeed (to do otherwise undermines the value of the
> validate endpoint). This makes me wonder about implementers, who might
> defensively program their configure(Map) method to implement the same
> checks. Therefore the contract should make clear that the Connect runtime
> guarantees that validate(Map) will be called before configure(Map).
>
> I don't really like the idea of implementing more-or-less the same default
> multiple times. Since these Transformation, Predicate etc will have a
> common contract wrt validate() and configure(), I wondered whether there
> was benefit in a common interface which Transformation etc could extend.
> It's a bit tricky because Connector and Converter are not Configurable.
> This was the best I could manage:
>
> ```
> interface ConfigValidatable {
> /**
> * Validate the given configuration values against the given
> configuration definitions.
> * This method will be called prior to the invocation of any
> initializer method, such as {@link Connector#initialize(ConnectorContext)},
> or {@link Configurable#configure(Map)} and should report any errors in the
> given configuration value using the errorMessages of the ConfigValues in
> the returned Config. If the Config returned by this method has no errors
> then the initializer method should not throw due to bad configuration.
> *
> * @param configDef the configuration definition, which may be null.
> * @param configs the provided configuration values.
> * @return The updated configuration information given the current
> configuration values
> *
> * @since 3.2
> */
> default Config validate(ConfigDef configDef, Map<String, String>
> configs) {
> List<ConfigValue> configValues = configDef.validate(smtConfigs);
> return new Config(configValues);
> }
>
> }
> ```
>
> Note that the configDef is passed in, leaving it to the runtime to call
> `thing.config()` to get the ConfigDef instance and validate whether it is
> allowed to be null or not. The subinterfaces could override validate() to
> define what the "initializer method" is in their case, and to indicate
> whether configDef can actually be null.
>
> To be honest, I'm not really sure this is better, but I thought I'd suggest
> it to see what others thought.
>
> Kind regards,
>
> Tom
>
> On Tue, Dec 21, 2021 at 6:46 PM Chris Egerton <chr...@confluent.io.invalid>
> wrote:
>
> > Hi Gunnar,
> >
> > Thanks, this looks great. I'm ready to cast a non-binding on the vote
> > thread when it comes.
> >
> > One small non-blocking nit: I like that you call out that the new
> > validation steps will take place when a connector gets registered or
> > updated. IMO this is important enough to be included in the "Public
> > Interfaces" section as that type of preflight check is arguably more
> > important than the PUT /connector-plugins/{name}/config/validate endpoint,
> > when considering that use of the validation endpoint is strictly opt-in,
> > but preflight checks for new connector configs are unavoidable (without
> > resorting to devious hacks like publishing directly to the config topic).
> > But this is really minor, I'm happy to +1 the KIP as-is.
> >
> > Cheers,
> >
> > Chris
> >
> > On Tue, Dec 21, 2021 at 8:43 AM Gunnar Morling
> > <gunnar.morl...@googlemail.com.invalid> wrote:
> >
> > > Hey Chris,
> > >
> > > Thanks a lot for reviewing this KIP and your comments! Some more answers
> > > inline.
> > >
> > > Am Di., 7. Dez. 2021 um 23:49 Uhr schrieb Chris Egerton
> > > <chr...@confluent.io.invalid>:
> > >
> > > > Hi Gunnar,
> > > >
> > > > Thanks for the KIP! The section on backwards compatibility is
> > especially
> > > > impressive and was enjoyable to read.
> > > >
> > >
> > > Excellent, that's great to hear!
> > >
> > > Overall I like the direction of the KIP (and in fact just ran into a
> > > > situation yesterday where it would be valuable). I only have one major
> > > > thought: could we add similar validate methods for the Converter and
> > > > HeaderConverter interfaces? With KIP-769 [1], it looks like we'll have
> > a
> > > > new Converter::config method, so if that makes it through, it should
> > be a
> > > > matter of just adding the same methods to those interfaces as well
> > > > (although we may want to be tolerant of null ConfigDef objects being
> > > > returned from HeaderConverter::config since the Connect framework has
> > not
> > > > been enforcing this requirement to date).
> > > >
> > >
> > > Yes, I think it's a good idea to expand the scope of the KIP to cover all
> > > these contracts. I have updated the KIP document accordingly.
> > >
> > > >
> > > > That aside, a few small nits:
> > > >
> > > > 1. The "This page is meant as a template" section can be removed :)
> > > > 2. The "Current Status" can be updated to "Under Discussion"
> > > > 3. Might want to add javadocs to the newly-proposed validate method
> > (I'm
> > > > assuming they'll largely mirror the ones for the existing
> > > > Connector::validate method, but we may also want to add a {@since} tag
> > or
> > > > some other information on which versions of Connect will leverage the
> > > > method).
> > > >
> > >
> > > Done.
> > >
> > > I will try and create a PR for this work in January next year.
> > >
> > > All the best,
> > >
> > > --Gunnar
> > >
> > > [1] -
> > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+APIs+to+list+all+plugins+and+retrieve+their+configuration+definitions#KIP769:ConnectAPIstolistallpluginsandretrievetheirconfigurationdefinitions-PublicInterfaces
> > > > (section labeled "Converter interface"
> > > >
> > > > Cheers,
> > > >
> > > > Chris
> > > >
> > > > On Wed, Nov 24, 2021 at 11:32 AM Gunnar Morling
> > > > <gunnar.morl...@googlemail.com.invalid> wrote:
> > > >
> > > > > Hey all,
> > > > >
> > > > > I would like to propose a KIP for Apache Kafka Connect which adds
> > > > > validation support for SMT-related configuration options:
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-802%3A+Validation+Support+for+Kafka+Connect+SMT+Options
> > > > >
> > > > > This feature allows users to make sure an SMT is configured correctly
> > > > > before actually putting a connector with that SMT in place.
> > > > >
> > > > > Any feedback, comments, and suggestions around this proposal will
> > > > > be greatly appreciated.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > --Gunnar
> > > > >
> > > >
> > >
> >
