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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
