Hi,
I've not received a reply from Gunnar since last month, so I'll pick
this KIP up.
Thanks,
Mickael
On Tue, Jun 18, 2024 at 6:01 PM Mickael Maison <mickael.mai...@gmail.com> wrote:
>
> 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
> > > > > >
> > > > >
> > > >
> > >
