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