Hi Arjun, Thank you for taking a look at this KIP.
1.a Yes this is exactly what this KIP is proposing. 1.b I prefer returning all plugins together instead of having different paths. 2. While we seem to agree it would be nice to expose worker plugins, I don't think we've reached a consensus on the right way to do it. For that reason, I'm dropping this feature from this KIP. We can address this issue in another KIP. I have updated the KIP. Thanks, Mickael On Mon, Dec 6, 2021 at 10:15 AM Mickael Maison <mickael.mai...@gmail.com> wrote: > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >