Thanks Randall, updated.
On Sun, Dec 5, 2021 at 6:05 AM Arjun Satish <arjun.sat...@gmail.com> wrote:
>
> hey folks,
>
> Thanks a lot for the KIP and the discussions. Here are a couple of thoughts
> (apologies if I missed some point in this thread).
>
> 1. it seems like we are changing the behavior of /connector-plugins by now
> returning the list of transforms, converters as well? This would break
> backward compatibility, and clients would need to track what version of
> Connect workers they are querying to keep their experience consistent. I
> wonder if we can keep the old behavior, with one of the following methods:
>
> 1a. We add an "expand" multi-query to the /connector-plugin endpoint like
> how it was done in KIP-465 [1]. This would preserve backward compatibility,
> as well as allow you to get consolidated output. Acceptable values for the
> new param could be: "all" (to show all plugins), or
> "connectors|transforms|predicates" that show a subset of the plugin types.
>
> 1b. Query params might get crummy to extend. Alternatively, we could have
> an additional fine-grained paths for each plugin:
>
> # to list all the source and sink plugins
> /connector-plugins/connectors
>
> # to list all the SMT plugins
> /connector-plugins/transformations
>
> # to list all the predicate plugins
> /connector-plugins/predicates
>
> # to list all the converter plugins
> /connector-plugins/converters
>
> # to list all the header converter plugins
> /connector-plugins/header-converters
>
> This allows us to cleanly add new types of plugins down the road, and
> naturally extend to displaying attributes of these objects. Configs, for
> example, can be accessed with
>
> http://localhost:8083/connector-plugins/transformations/Cast$Value/config
>
> A migration path (for both the suggestions above) would be to add a
> deprecation notice on the current behavior of /connector-plugins (where,
> after this PR is merged, both /connector-plugins and
> /connector-plugins/connectors return the same output). And starting from
> the next major version release, /connector-plugins redirects to
> /connector-plugins/connectors (making it super easy for clients to catch
> up).
>
> 2. Regarding /worker-plugins, I agree that it is useful to have the REST
> API inform the state of the REST extensions, and Config Providers. However,
> users of the Connect clusters will rarely need to know how a config
> provider or REST extension is configured to construct data pipelines. It's
> mostly an admin task, AFAICT. Also, we have also seen these problems in
> Connect clusters where one host is somewhat differently set up than other
> (contents of plugin path, classpath, local worker configuration files, jvm
> versions, GC settings) that surface as issues in Connector behavior.
> Overall, I'm wondering if we should just tackle this problem in a separate
> KIP, that evaluates all of these cases. All of this might nicely fall into
> under the /admin endpoint and allow us to expose detailed info on the state
> of the worker (maybe a /admin/worker/ that returns all of this info).
>
> Thanks,
>
> [1]
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-465%3A+Add+Consolidated+Connector+Endpoint+to+Connect+REST+API
>
>
> On Sat, Dec 4, 2021 at 10:29 AM Randall Hauch <rha...@gmail.com> wrote:
>
> > Thanks, Mickael. Can we call out these as literals? For example:
> >
> > > The response structure of the objects in the array remain unchanged, but
> > this proposal does add "converter", "header_converter" and "transformation"
> > to the allowed values for the "type" field, in addition to the existing
> > "sink" and "source" values.
> >
> > Best regards,
> >
> > Randall
> >
> > On Sat, Dec 4, 2021 at 4:58 AM Mickael Maison <mickael.mai...@gmail.com>
> > wrote:
> > >
> > > Hi Randall,
> > >
> > > Sure. The types are sink, source, converter, header_converter,
> > > transformation and predicate. I've updated the KIP.
> > >
> > > Thanks,
> > > Mickael
> > >
> > > On Fri, Dec 3, 2021 at 10:14 PM Randall Hauch <rha...@gmail.com> wrote:
> > > >
> > > > One more thing, inline below:
> > > >
> > > > Randall
> > > >
> > > > On Fri, Dec 3, 2021 at 10:38 AM Mickael Maison <
> > mickael.mai...@gmail.com> wrote:
> > > > >
> > > > > Thanks Randall for the feedback!
> > > > >
> > > > > 1. Done
> > > > >
> > > > > 2. Yes that makes sense. I've updated the KIP
> > > > >
> > > > > 3.a I've updated to explicitly mention the plugin can be provided
> > > > > using the fully qualified class or the alias.
> > > > > 3.b Only concrete classes will be returned. I made the mistake when I
> > > > > crafted the examples by hand. Fixed
> > > >
> > > > Can we also enumerate all of the values that can be included in the
> > > > "type" field in the response from the "GET
> > > > /connector-plugins?connectorsOnly=false"?
> > > >
> > > > >
> > > > > 4. If a plugin does not override config(), the response is an empty
> > > > > array. I've updated the KIP
> > > > >
> > > > > 5. I have updated the compatibility section with more details
> > > > >
> > > > > 6. Can you clarify what you mean? KIP-507 was for securing internal
> > > > > REST endpoints.
> > > > >
> > > > > 7. I don't think it is necessary. I think the use case Chris
> > mentioned
> > > > > could be useful for admins. I'm interested in exposing the
> > > > > connector-level plugins and their configs. I thought it was easy to
> > > > > add an API for worker plugins while working in this area but if there
> > > > > isn't a strong consensus, I'm happy to drop it.
> > > > >
> > > > > Mickael
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Wed, Dec 1, 2021 at 5:39 PM Randall Hauch <rha...@gmail.com>
> > wrote:
> > > > > >
> > > > > > Thanks for the KIP, Mickael. And thanks to everyone else for the
> > good
> > > > > > discussion so far.
> > > > > >
> > > > > > Overall this is pretty good, but I do have a few
> > questions/comments:
> > > > > > 1. Can we include the HTTP method/verb in each of the endpoints in
> > the
> > > > > > "Public Interfaces" section?
> > > > > > 2. The sample output for `GET
> > /connector-plugins?connectorsOnly=false`
> > > > > > includes a `version` field for the connector plugin, but not for
> > the
> > > > > > transform or converter plugins. Neither the `Transformation` or
> > > > > > `Converter` interfaces have a version nor implement the `Versioned`
> > > > > > interface, so should we add the `Versioned` interface to the
> > > > > > `Transformation` and `Converter` interfaces and add a default
> > method
> > > > > > that returns "unknown"? Yes, we could do that in a separate KIP,
> > but I
> > > > > > think it's appropriate to do it in this KIP for a few reasons.
> > First,
> > > > > > we're already modifying the `Converter` interface in this KIP and
> > it
> > > > > > would be quite straightforward to also extend the `Versioned`
> > > > > > interface and add a default method; then doing that for
> > > > > > `Transformation` would also be relatively straightforward. Second,
> > > > > > this KIP provides the justification for adding a version to the
> > > > > > `Transformation` and `Converter` interfaces.
> > > > > > 3. In the "Public Interfaces" section, there are two
> > inconsistencies
> > > > > > between the `GET /connector-plugins` and `GET
> > > > > > /connector-plugins/<plugin>/config` methods. Both will make it
> > > > > > difficult to use the second method, since the input to the second
> > > > > > method is not directly found in the response from the first method:
> > > > > > a. The example for `GET /connector-plugins/Cast$Value/config` uses
> > > > > > short name, or what Connect often refers to as the alias,
> > "Cast$Value"
> > > > > > for the plugin, rather than the fully-qualified name
> > > > > > "org.apache.kafka.connect.transforms.Cast$Value". The KIP should
> > > > > > specify whether it will support both the shortened name and the
> > > > > > fully-qualified name, or whether only one of these will be
> > supported.
> > > > > > Not supporting the fully-qualified name will make it impossible to
> > use
> > > > > > the response from the first method as an input to the second.
> > > > > > a. The example for the list method returns the `Transformation`
> > base
> > > > > > and abstract classes (e.g.,
> > > > > > "org.apache.kafka.connect.transforms.Cast") rather than the
> > concrete
> > > > > > classes (e.g., "org.apache.kafka.connect.transforms.Cast$Value" and
> > > > > > "org.apache.kafka.connect.transforms.Cast$Key"), while the `GET
> > > > > > /connector-plugins/<plugin>/config` will require the concrete
> > class.
> > > > > > 4. The "Public Interfaces" section's description of the `GET
> > > > > > /connector-plugins/<plugin>/config` method should probably specify
> > the
> > > > > > expected behavior for `Converter` implementations that don't
> > override
> > > > > > the new `config()` method.
> > > > > > 5. The "Compatibility, Deprecation, and Migration Plan" section
> > should
> > > > > > describe whether older `Converter` implementations (compiled with
> > > > > > older versions of the Connect API without Converter extending
> > > > > > Configurable) will be able to be installed into newer Connect
> > > > > > runtimes, and what the behavior will be for the `GET
> > > > > > /connector-plugins/<plugin>/config` method. (Although my suggestion
> > > > > > for #5 will partially address the latter, we still need to talk
> > about
> > > > > > it in terms of compatibility.)
> > > > > > 6. Can we add a small section addressing the security implications
> > of
> > > > > > these new APIs? KIP-507 [1] applied to APIs that existed at the
> > time,
> > > > > > and this KIP should at least mention how it aligns with that
> > > > > > mechanism.
> > > > > > 7. Do we really think adding `GET /worker-plugins` is necessary?
> > While
> > > > > > I understand the reasoning, I'm less convinced that it's worth
> > adding
> > > > > > to the public API. First, an administrator still has to modify the
> > > > > > worker config for all workers to take advantage of that. Second,
> > > > > > adding this method increases the surface area and therefore risk of
> > > > > > additional attack vectors, despite not really being able to use the
> > > > > > resulting information in any other part of the API.
> > > > > >
> > > > > > Best regards,
> > > > > >
> > > > > > Randall
> > > > > >
> > > > > > [1]
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-507%3A+Securing+Internal+Connect+REST+Endpoints
> > > > > >
> > > > > >
> > > > > > On Wed, Dec 1, 2021 at 7:52 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
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> >
