Hi Mickael,
Looks good to me! +1 non-binding when the vote thread opens :)
Cheers,
Chris
On Wed, Dec 1, 2021 at 8:53 AM Mickael Maison <mickael.mai...@gmail.com>
wrote:
> Hi Chris,
>
> Thanks again for the feedback! I've updated the KIP based on our last
> discussions. I've decided to include the new endpoint for worker
> plugins.
>
> 1. Yes I agree, it's best to gate the new behavior.
> 2. Yes, it was a remnant from the original proposal. I've now removed
> the location field.
>
> Thanks,
> Mickael
>
> On Tue, Nov 30, 2021 at 3:22 AM Chris Egerton
> <chr...@confluent.io.invalid> wrote:
> >
> > Hi Mickael,
> >
> > I think that's a great idea! I especially like how we can establish the
> > expectation that any plugin type that appears in the response from the
> GET
> > /connector-plugins endpoint will have a corresponding GET
> > /connector-plugins/<type>/config endpoint, but (if we decide to add them
> in
> > the future), worker plugins won't be expected to expose this kind of
> > information and the different root path helps give a decent hint about
> this.
> >
> > I also like the choice to return an empty ConfigDef from
> Converter::config
> > instead of null.
> >
> > Two things come to mind:
> >
> > 1. We may want to gate this behind a URL query parameter (maybe something
> > like "connectorsOnly") that defaults to the old behavior in order to
> avoid
> > breaking existing tools such as programmatic UIs that use the endpoint
> > today to discover the connectors that can be created by the user. We can
> > even plan to change the default for that parameter to the newly-proposed
> > behavior in the next major release, which should give people enough time
> to
> > either adapt to the expanded response format or add the query parameter
> to
> > their tooling.
> >
> > 2. The existing GET /connector-plugins endpoint doesn't contain
> information
> > on the location of the plugin on the worker's file system. Do you think
> we
> > should still include this info in the new response format? Correct me if
> > I'm wrong but it seems it may have been proposed originally to help
> prevent
> > already-addressed bugs in Connect classloading from striking; all else
> > equal, I'd personally err on the side of leaving this info out or at
> least
> > reducing permitted values for it to just "classpath" or "plugin path" in
> > order to avoid leaking worker file paths into the REST API, which might
> > bother super security-conscious users.
> >
> > Cheers,
> >
> > Chris
> >
> > On Mon, Nov 29, 2021 at 5:52 AM Mickael Maison <mickael.mai...@gmail.com
> >
> > wrote:
> >
> > > Hi Chris,
> > >
> > > Yes to keep compatibility we want a default implementation for
> > > Converter.configs(), I've updated the KIP.
> > >
> > > Regarding worker plugins, the use case you described seems valuable.
> > > I'd prefer not mixing worker and connector plugins on the same
> > > endpoint but I agree using /plugins and /worker-plugins could be
> > > confusing.
> > >
> > > One alternative is to expose all connector-level plugins via the
> > > existing /connector-plugins endpoint. In that case, we'd need to keep
> > > the current JSON schema and not group plugins by type. As the current
> > > schema already has a type field for each entry, we'll still be able to
> > > tell them apart. Then we can have /worker-plugins and a relatively
> > > clean API. What do you think?
> > >
> > > Thanks,
> > > Mickael
> > >
> > > On Sun, Nov 28, 2021 at 8:21 PM Chris Egerton
> > > <chr...@confluent.io.invalid> wrote:
> > > >
> > > > Hi Mickael,
> > > >
> > > > I think one potential use case for exposing worker-level plugins is
> that
> > > it
> > > > may make it easier to determine whether a worker is set up correctly
> (the
> > > > current alternative requires looking through log files and can be a
> > > little
> > > > tedious), and might even make it possible to automatically identify
> > > > discrepancies within a cluster by diffing the contents of that
> endpoint
> > > > across each worker. But I don't think this has to be addressed by the
> > > > current KIP; the only thing that bothers me a little is that
> "plugins" is
> > > > generic and it may confuse people down the road if we add an
> endpoint for
> > > > worker-level plugins ("why is one just called 'plugins' and the
> other one
> > > > is 'worker-plugins'?"). Probably not worth blocking on, though.
> > > >
> > > > Agreed that the suggestion for improved validation should be made on
> the
> > > > KIP-802 thread.
> > > >
> > > > I also noticed that the newly-proposed config method for the
> Converter
> > > > interface doesn't have a default implementation, making it
> > > > backwards-incompatible. Should we add a default implementation that
> > > returns
> > > > either null or an empty ConfigDef?
> > > >
> > > > Cheers,
> > > >
> > > > Chris
> > > >
> > > > On Fri, Nov 26, 2021 at 8:35 AM Mickael Maison <
> mickael.mai...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > > > Hi Chris,
> > > > >
> > > > > 1. If we want to expose worker plugins, I think we should do it
> via a
> > > > > separate endpoint. But to be honest, I'm not even sure I see strong
> > > > > use cases for exposing them as they are either enabled or not and
> > > > > can't be changed at runtime. So I'd prefer to stick to "connector
> > > > > level" plugins in this KIP. Let me now if you have use cases, I'm
> open
> > > > > to reconsider this choice.
> > > > > I'll add that in the rejected alternatives section for now
> > > > >
> > > > > 2. I remembered seeing issues in the past with multiple plugin.path
> > > > > entries but I tried today and I was able to mix and match plugins
> from
> > > > > different paths. So my bad for getting confused.
> > > > > Then I agree, it makes more sense to group them by plugin type.
> > > > >
> > > > > 3. Yes this should be covered in KIP-802:
> > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-802%3A+Validation+Support+for+Kafka+Connect+SMT+Options
> > > > >
> > > > > 4. No particular reason. We can support both formats like today.
> I've
> > > > > updated the KIP
> > > > >
> > > > > Thanks,
> > > > > Mickael
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Nov 23, 2021 at 6:40 PM Chris Egerton
> > > > > <chr...@confluent.io.invalid> wrote:
> > > > > >
> > > > > > Hi Mickael,
> > > > > >
> > > > > > I think the increase in scope here is great and the added value
> > > certainly
> > > > > > justifies the proposed changes. I have some thoughts but overall
> I
> > > like
> > > > > the
> > > > > > direction this is going in now.
> > > > > >
> > > > > > 1. The new /plugins endpoint is described as containing "all
> plugins
> > > that
> > > > > > are Connectors, Transformations, Converters, HeaderConverters and
> > > > > > Predicates". So essentially, it looks like we want to expose all
> > > plugins
> > > > > > that are configured on a per-connector basis, but exclude plugins
> > > that
> > > > > are
> > > > > > configured on a per-worker basis (such as config providers and
> REST
> > > > > > extensions). Do you think it may be valuable to expose
> information on
> > > > > > worker-level plugins as well?
> > > > > >
> > > > > > 2. The description for the new /plugins endpoint also states that
> > > > > "Plugins
> > > > > > will be grouped by plugin.path. This will make it clear to users
> > > what's
> > > > > > available to use as it's not possible to use a Connector from one
> > > path
> > > > > with
> > > > > > Transformations from another.". Is this true? I thought that
> > > Connect's
> > > > > > classloading made it possible to package
> > > > > > converters/transformations/predicates completely independently
> from
> > > each
> > > > > > other, and to reference them from also-independently-packaged
> > > connectors.
> > > > > > If it turns out that this is the case, could we consider
> > > restructuring
> > > > > the
> > > > > > response to be grouped by plugin type instead of by classloader?
> > > There's
> > > > > > also the ungrouped format proposed in KIP-494 (
> > > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=120740150
> > > > > )
> > > > > > which we might consider as well.
> > > > > >
> > > > > > 3. I think this can be left for a follow-up KIP if necessary,
> but I'm
> > > > > > curious about your thoughts on adding new validate methods to all
> > > > > > connector-level plugins that can be used similarly to how the
> > > existing
> > > > > > Connector::validate method (
> > > > > >
> > > > >
> > >
> https://github.com/apache/kafka/blob/1e0916580f16b99b911b0ed36e9740dcaeef520e/connect/api/src/main/java/org/apache/kafka/connect/connector/Connector.java#L131-L146
> > > > > )
> > > > > > is used. This would allow for plugins to perform validation
> that's
> > > more
> > > > > > sophisticated than what the ConfigDef is capable of, such as
> > > validating
> > > > > > combinations of properties like a hostname and credentials for
> > > reaching
> > > > > it.
> > > > > > I know that at least Confluent's Avro, protobuf, and JSON schema
> > > > > converters
> > > > > > would benefit from this kind of feature. It's a little
> tangential to
> > > this
> > > > > > KIP (which at the moment is about discovering plugins and their
> > > > > > configuration surfaces, as opposed to validating them), but I
> > > figured I'd
> > > > > > ask since we're going to be expanding the Converter interface
> and it
> > > may
> > > > > be
> > > > > > useful to tackle this while we're in the neighborhood.
> > > > > >
> > > > > > 4. The description for the new /plugins/<type>/<name>/configdef
> > > endpoint
> > > > > > states that "Name must be the fully qualified class name of the
> > > plugin".
> > > > > > Any reason not to also support aliases (e.g.,
> > > "FileStreamSinkConnector"
> > > > > or
> > > > > > "FileStreamSink" instead of
> > > > > > "org.apache.kafka.connect.file.FileStreamSinkConnector")?
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Chris
> > > > > >
> > > > > > On Tue, Nov 23, 2021 at 12:07 PM Mickael Maison <
> > > > > mickael.mai...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Thanks all for the feedback!
> > > > > > >
> > > > > > > Chris,
> > > > > > > I agree that fixing the current endpoint helps a lot. Thanks
> for
> > > > > > > raising these JIRAs and submitting a PR!
> > > > > > > However thinking about the issue further, I decided to expand
> the
> > > > > > > scope of the KIP to cover all user-visible plugins.
> > > > > > > In practice, users want to know about all available plugins not
> > > only
> > > > > > > connectors. This includes transformations, converters,
> > > > > > > header_converters and predicates. As we also want to retrieve
> > > > > > > configdef for these too, I think it makes sense to introduce a
> new
> > > > > > > endpoint to do so. Alongside we obviously need a new endpoint
> for
> > > > > > > listing all plugins.
> > > > > > >
> > > > > > > Gunnar,
> > > > > > > I took a look at exposing valid values via the API. I think the
> > > issue
> > > > > > > is that Validators don't expose a way to retrieve valid values.
> > > > > > > Changing validators will have an impact on all components so
> I'd
> > > > > > > prefer to address this requirement in a separate KIP. I agree
> this
> > > > > > > would be an interesting improvement and I'd happy to write a
> KIP
> > > for
> > > > > > > it too.
> > > > > > >
> > > > > > >
> > > > > > > I have updated the KIP accordingly. Let me know if you have
> further
> > > > > > > feedback.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Mickael
> > > > > > >
> > > > > > > On Tue, Nov 16, 2021 at 9:31 PM Gunnar Morling
> > > > > > > <gunnar.morl...@googlemail.com.invalid> wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I'm +1 for adding a GET endpoint for obtaining config
> > > definitions. It
> > > > > > > > always felt odd to me that one has to issue a PUT for that
> > > purpose.
> > > > > If
> > > > > > > > nothing else, it'd be better in terms of discoverability of
> the
> > > KC
> > > > > REST
> > > > > > > API.
> > > > > > > >
> > > > > > > > One additional feature request I'd have is to expose the
> valid
> > > enum
> > > > > > > > constants for enum-typed options. That'll help to display the
> > > values
> > > > > in a
> > > > > > > > drop-down or via radio buttons in a UI, give us tab
> completion in
> > > > > kcctl,
> > > > > > > > etc.
> > > > > > > >
> > > > > > > > Best,
> > > > > > > >
> > > > > > > > --Gunnar
> > > > > > > >
> > > > > > > >
> > > > > > > > Am Di., 16. Nov. 2021 um 16:31 Uhr schrieb Chris Egerton
> > > > > > > > <chr...@confluent.io.invalid>:
> > > > > > > >
> > > > > > > > > Hi Viktor,
> > > > > > > > >
> > > > > > > > > It sounds like there are three major points here in favor
> of a
> > > new
> > > > > GET
> > > > > > > > > endpoint for connector config defs.
> > > > > > > > >
> > > > > > > > > 1. You cannot issue a blank ("dummy") request for sink
> > > connectors
> > > > > > > because a
> > > > > > > > > topic list/topic regex has to be supplied (otherwise the
> PUT
> > > > > endpoint
> > > > > > > > > returns a 500 response)
> > > > > > > > > 2. A dummy request still triggers custom validations by the
> > > > > connector,
> > > > > > > > > which may be best to avoid if we know for sure that the
> config
> > > > > isn't
> > > > > > > worth
> > > > > > > > > validating yet
> > > > > > > > > 3. It's more ergonomic and intuitive to be able to issue a
> GET
> > > > > request
> > > > > > > > > without having to give a dummy connector config
> > > > > > > > >
> > > > > > > > > With regards to 1, this is actually a bug in Connect (
> > > > > > > > > https://issues.apache.org/jira/browse/KAFKA-13327) with a
> fix
> > > > > already
> > > > > > > > > implemented and awaiting committer review (
> > > > > > > > > https://github.com/apache/kafka/pull/11369). I think it'd
> be
> > > > > better to
> > > > > > > > > focus on fixing this bug in general instead of
> implementing a
> > > new
> > > > > REST
> > > > > > > > > endpoint in order to allow people to work around it.
> > > > > > > > >
> > > > > > > > > With regards to 2, this is technically possible but I'm
> unsure
> > > > > it'd be
> > > > > > > too
> > > > > > > > > common out in the wild given that most validations that
> could
> > > be
> > > > > > > expensive
> > > > > > > > > would involve things like connecting to a database,
> checking
> > > if a
> > > > > cloud
> > > > > > > > > storage bucket exists, etc., none of which are possible
> without
> > > > > some
> > > > > > > > > configuration properties from the user (db hostname, bucket
> > > name,
> > > > > > > etc.).
> > > > > > > > >
> > > > > > > > > With regards to 3, I do agree that it'd be easier for
> people
> > > > > designing
> > > > > > > UIs
> > > > > > > > > to have a GET API to work against. I'm just not sure it's
> > > worth the
> > > > > > > > > additional implementation, testing, and maintenance
> burden. If
> > > it
> > > > > were
> > > > > > > > > possible to issue a PUT request without unexpected 500s for
> > > invalid
> > > > > > > > > configs, would that suffice? AFAICT it'd basically be as
> > > simple as
> > > > > > > issuing
> > > > > > > > > a PUT request with a dummy body consisting of nothing
> except
> > > the
> > > > > > > connector
> > > > > > > > > class (which at this point we might even make unnecessary
> and
> > > just
> > > > > > > > > automatically replace with the connector class from the
> URL)
> > > and
> > > > > then
> > > > > > > > > filtering the response to just grab the "definition" field
> of
> > > each
> > > > > > > element
> > > > > > > > > in the "configs" array in the response.
> > > > > > > > >
> > > > > > > > > Cheers,
> > > > > > > > >
> > > > > > > > > Chris
> > > > > > > > >
> > > > > > > > > On Tue, Nov 16, 2021 at 9:52 AM Viktor Somogyi-Vass <
> > > > > > > > > viktorsomo...@gmail.com>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Folks,
> > > > > > > > > >
> > > > > > > > > > I too think this would be a very useful feature. Some of
> our
> > > > > > > management
> > > > > > > > > > applications would provide a wizard for creating
> connectors.
> > > In
> > > > > this
> > > > > > > > > > scenario the user basically would fill out a sample
> > > configuration
> > > > > > > > > generated
> > > > > > > > > > by the UI which would send it back to Connect for
> validation
> > > and
> > > > > > > > > eventually
> > > > > > > > > > create a new connector. The first part of this workflow
> can
> > > be
> > > > > > > enhanced
> > > > > > > > > if
> > > > > > > > > > we had an API that can return the configuration
> definition
> > > of the
> > > > > > > given
> > > > > > > > > > type of connector as the UI application would be able to
> > > > > generate a
> > > > > > > > > sample
> > > > > > > > > > for the user based on that (nicely drawn diagram:
> > > > > > > > > > https://imgur.com/a/7S1Xwm5).
> > > > > > > > > > The connector-plugins/{connectorType}/config/validate API
> > > > > essentially
> > > > > > > > > works
> > > > > > > > > > and returns the data that we need, however it is a HTTP
> PUT
> > > API
> > > > > that
> > > > > > > is a
> > > > > > > > > > bit unintuitive for a fetch-like functionality and also
> > > > > functionally
> > > > > > > > > > different as it validates the given (dummy) request. In
> case
> > > of
> > > > > sink
> > > > > > > > > > connectors one would need to also provide a topic name.
> > > > > > > > > >
> > > > > > > > > > A suggestion for the KIP: I think it can be useful to
> return
> > > the
> > > > > > > config
> > > > > > > > > > groups and the connector class' name similarly to the
> > > validate
> > > > > API
> > > > > > > just
> > > > > > > > > in
> > > > > > > > > > case any frontend needs them (and also the response
> would be
> > > more
> > > > > > > like
> > > > > > > > > the
> > > > > > > > > > validate API but simpler).
> > > > > > > > > >
> > > > > > > > > > Viktor
> > > > > > > > > >
> > > > > > > > > > On Fri, Aug 20, 2021 at 4:51 PM Ryanne Dolan <
> > > > > ryannedo...@gmail.com>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > I think it'd be worth adding a GET version, fwiw.
> Could be
> > > the
> > > > > same
> > > > > > > > > > handler
> > > > > > > > > > > with just a different spelling maybe.
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Aug 20, 2021, 7:44 AM Mickael Maison <
> > > > > > > mickael.mai...@gmail.com
> > > > > > > > > >
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Chris,
> > > > > > > > > > > >
> > > > > > > > > > > > You're right, you can achieve the same functionality
> > > using
> > > > > the
> > > > > > > > > > > > existing validate endpoint.
> > > > > > > > > > > > In my mind it was only for validation once you have
> > > build a
> > > > > > > > > > > > configuration but when used with an empty
> configuration,
> > > it
> > > > > > > basically
> > > > > > > > > > > > serves the same purpose as the proposed new endpoint.
> > > > > > > > > > > >
> > > > > > > > > > > > I think it's a bit easier to use a GET endpoint but I
> > > don't
> > > > > > > think it
> > > > > > > > > > > > really warrants a different endpoint.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Aug 19, 2021 at 2:56 PM Chris Egerton
> > > > > > > > > > > > <chr...@confluent.io.invalid> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi Mickael,
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'm wondering about the use case here. The
> motivation
> > > > > section
> > > > > > > > > states
> > > > > > > > > > > that
> > > > > > > > > > > > > "Connect does not provide a way to see what
> > > configurations
> > > > > a
> > > > > > > > > > connector
> > > > > > > > > > > > > requires. Instead users have to go look at the
> > > connector
> > > > > > > > > > documentation
> > > > > > > > > > > or
> > > > > > > > > > > > > in the worst case, look directly at the connector
> > > source
> > > > > > > code.",
> > > > > > > > > and
> > > > > > > > > > > that
> > > > > > > > > > > > > with this KIP, "users will be able to discover the
> > > required
> > > > > > > > > > > > configurations
> > > > > > > > > > > > > for connectors installed in a Connect cluster" and
> > > "tools
> > > > > will
> > > > > > > be
> > > > > > > > > > able
> > > > > > > > > > > to
> > > > > > > > > > > > > generate wizards for configuring and starting
> > > connectors".
> > > > > > > > > > > > >
> > > > > > > > > > > > > Does the existing "PUT
> > > > > > > > > > > > /connector-plugins/{connector-type}/config/validate"
> > > > > > > > > > > > > endpoint not address these points? What will the
> > > > > newly-proposed
> > > > > > > > > > > endpoint
> > > > > > > > > > > > > allow users to do that they will not already be
> able
> > > to do
> > > > > > > with the
> > > > > > > > > > > > > existing endpoint?
> > > > > > > > > > > > >
> > > > > > > > > > > > > Cheers,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Chris
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Aug 19, 2021 at 9:20 AM Mickael Maison <
> > > > > > > > > > > mickael.mai...@gmail.com
> > > > > > > > > > > > >
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I've created KIP-769 to expose connector
> > > configuration
> > > > > > > > > definitions
> > > > > > > > > > in
> > > > > > > > > > > > > > the Connect API
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+API+to+retrieve+connector+configuration+definitions
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Please take a look and let me know if you have
> any
> > > > > feedback.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks
> > > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
