You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Gunnar Morling <gu...@googlemail.com.INVALID> on 2021/11/24 16:31:38 UTC
[DISCUSS] KIP-802: Validation Support for Kafka Connect SMT Options
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
Re: [DISCUSS] KIP-802: Validation Support for Kafka Connect SMT Options
Posted by Tom Bentley <tb...@redhat.com>.
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 <ch...@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
> <gu...@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
> > <ch...@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
> > > <gu...@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
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-802: Validation Support for Kafka Connect SMT Options
Posted by Chris Egerton <ch...@confluent.io.INVALID>.
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
<gu...@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
> <ch...@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
> > <gu...@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
> > >
> >
>
Re: [DISCUSS] KIP-802: Validation Support for Kafka Connect SMT Options
Posted by Gunnar Morling <gu...@googlemail.com.INVALID>.
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
<ch...@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
> <gu...@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
> >
>
Re: [DISCUSS] KIP-802: Validation Support for Kafka Connect SMT Options
Posted by Chris Egerton <ch...@confluent.io.INVALID>.
Hi Gunnar,
Thanks for the KIP! The section on backwards compatibility is especially
impressive and was enjoyable to read.
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).
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).
[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
<gu...@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
>