I'd like to keep the scope pretty tight -- getting SMTs is the real motivation here.
If you think deprecation of the existing `/connector-plugins` endpoint necessitates expanding the scope to re-thinking a new top-level API and the implications for every subtype, then I think the right move here is to not deprecate. I would update KIP to explain why we did not deprecate, and leave the door open as future work. WDYT? On Tue, Jun 8, 2021 at 11:52 AM Konstantine Karantasis <konstant...@confluent.io.invalid> wrote: > That's a valid point based on what the implementation is today. But this > new endpoint aims to be an extension and a superset in terms of > functionality of the existing `/connector-plugins` endpoint. > > This last discussion and the fact that the endpoint `/plugins/connector` > that you suggest is almost synonymous to `/connector-plugins' makes me > think that we might have not thought through how this new endpoint will > evolve and look as a top level endpoint for the various plugin types. > > Konstantine > > > > On Mon, Jun 7, 2021 at 5:02 PM Cyrus Vafadari <cvafad...@gmail.com> wrote: > > > Since it is just two endpoints, I agree it's fine to move them over. > > > > However, the endpoint you suggest would imply that someone could attempt > to > > validate the configs of an SMT or Header Converter, since it doesn't > > specify the plugin's type. So if you agree I'll update the proposal to > move > > the PUT endpoint to > > > > PUT /plugins/connector/{connector-type}/config/validate > > > > so we don't have to return 400s for all the other types for which > > validation is not supported. > > > > Does that sound good to you? > > > > > > On Mon, Jun 7, 2021 at 4:49 PM Konstantine Karantasis > > <konstant...@confluent.io.invalid> wrote: > > > > > Hi Cyrus, > > > > > > Given that `/connector-plugins` is a top level endpoint, I think the > > > suggestion to deprecate `/connector-plugins` should affect all the > nested > > > endpoints. > > > > > > This should be straightforward here (in terms of KIP changes but also > > > implementation), because the endpoints are only two: > > > > > > GET /connector-plugins > > > and > > > PUT /connector-plugins/{connector-type}/config/validate > > > > > > I find deprecating both of them in favor of: > > > GET /plugins > > > and > > > PUT /plugins/{connector-type}/config/validate > > > > > > more clear and aligned with how we want the new endpoint to look like. > > > > > > Wdyt? > > > > > > Konstantine > > > > > > > > > On Mon, Jun 7, 2021 at 4:18 PM Cyrus Vafadari <cvafad...@gmail.com> > > wrote: > > > > > > > I wasn't trying to suggest deprecating all verbs and endpoints > prefixed > > > > with `/connector-plugins`, just the `GET: /connector-plugins` as I > > > detailed > > > > in the KIP. Maybe I'm not understanding your recommendation, or > > > there's a > > > > way I can make the KIP text clearer? Or do you disagree this is the > > right > > > > move tactically? I thought my text was sufficient: "it will be > > redundant > > > to > > > > this new, more generally useful endpoint." > > > > > > > > On Mon, Jun 7, 2021 at 12:56 PM Konstantine Karantasis > > > > <konstant...@confluent.io.invalid> wrote: > > > > > > > > > Thanks for the response Cyrus. > > > > > > > > > > If we are suggesting deprecation of `/connector-plugins` in favor > of > > > > > `/plugins` I think we should mention explicitly that the new > endpoint > > > > > covers the existing functionality precisely, including the > validation > > > > > endpoint for connector plugins. > > > > > I'd recommend extending the spec to refer to this, adding an > example > > to > > > > > make this clear and mentioning that the two endpoints will be > > > equivalent > > > > > with respect to existing functionality in the upgrade section. > > > > > > > > > > Konstantine > > > > > > > > > > On Mon, Jun 7, 2021 at 10:57 AM Cyrus Vafadari < > cvafad...@gmail.com> > > > > > wrote: > > > > > > > > > > > I will move to vote > > > > > > > > > > > > On Mon, Jun 7, 2021 at 10:44 AM Cyrus Vafadari < > > cvafad...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > Thanks for the feedback, > > > > > > > > > > > > > > I added an example request/response for SMTs, and I thought > about > > > > your > > > > > > > suggestion re:deprecation and am now explicitly proposing to > mark > > > the > > > > > > > existing endpoint as deprecated, though I don't anticipate the > > need > > > > to > > > > > > > remove it will arise any time soon! > > > > > > > > > > > > > > Cyrus > > > > > > > > > > > > > > On Fri, Jun 4, 2021 at 7:35 PM Konstantine Karantasis > > > > > > > <konstant...@confluent.io.invalid> wrote: > > > > > > > > > > > > > >> Hi Cyrus. > > > > > > >> > > > > > > >> The proposal looks good and I like the API spec definition the > > way > > > > > it's > > > > > > >> presented. > > > > > > >> > > > > > > >> Having said that, a few examples that would list the request > > type > > > > and > > > > > > >> body, > > > > > > >> the returned status and the json response would be nice too, > > > > following > > > > > > the > > > > > > >> tradition of other KIPs. > > > > > > >> > > > > > > >> See > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-745%3A+Connect+API+to+restart+connector+and+tasks > > > > > > >> for a recent example. > > > > > > >> > > > > > > >> I also see there's no mention regarding the future of the > > current > > > > > > >> `/connector-plugins` endpoint or any deprecation plans. > > > > > > >> > > > > > > >> I think we should make our intentions clear in the KIP itself > > > > > > >> which introduces the new endpoint as a superset of the old > one, > > > > beyond > > > > > > the > > > > > > >> discussion in this thread, for future readers. > > > > > > >> Also, I think it's useful to keep in mind that deprecation > > doesn't > > > > > > >> necessarily mean imminent removal. The next opportunity to > > remove > > > > this > > > > > > end > > > > > > >> point would be the next major release at the earliest. > > > > > > >> > > > > > > >> Regards, > > > > > > >> Konstantine > > > > > > >> > > > > > > >> > > > > > > >> On Mon, Apr 19, 2021 at 10:13 AM Cyrus Vafadari < > > > > cvafad...@gmail.com> > > > > > > >> wrote: > > > > > > >> > > > > > > >> > Thanks! Anyone else from the community with final thoughts > > > before > > > > > > going > > > > > > >> to > > > > > > >> > vote? > > > > > > >> > > > > > > > >> > On Mon, Apr 19, 2021 at 4:16 AM Tom Bentley < > > > tbent...@redhat.com> > > > > > > >> wrote: > > > > > > >> > > > > > > > >> > > Hi Cyrus, > > > > > > >> > > > > > > > > >> > > That seems reasonable to me. > > > > > > >> > > > > > > > > >> > > On Thu, Apr 15, 2021 at 6:44 PM Cyrus Vafadari < > > > > > cvafad...@gmail.com > > > > > > > > > > > > > >> > > wrote: > > > > > > >> > > > > > > > > >> > > > What do you think of "type" field remaining in, and > > > returning > > > > > > >> > > > "transformation", "converter" or whatever type it is. > This > > > way > > > > > the > > > > > > >> > schema > > > > > > >> > > > remains consistent, and you can programmatically > > understand > > > > what > > > > > > >> > plugins > > > > > > >> > > > are returned on the holistic "GET /plugins" endpoint? It > > > will > > > > be > > > > > > >> > slightly > > > > > > >> > > > redundant in the case where you specify the plugin-type > > as a > > > > > path > > > > > > >> > > > parameter. > > > > > > >> > > > > > > > > > >> > > > On Thu, Apr 15, 2021 at 1:13 PM Tom Bentley < > > > > > tbent...@redhat.com> > > > > > > >> > wrote: > > > > > > >> > > > > > > > > > >> > > > > Hi Cyrus, > > > > > > >> > > > > > > > > > > >> > > > > Re 2: A very minor thing but while type=source|sink > for > > a > > > > > > >> connector, > > > > > > >> > it > > > > > > >> > > > > doesn't makes sense for the other plugin types, but so > > the > > > > > json > > > > > > >> for > > > > > > >> > > those > > > > > > >> > > > > plugins should omit that property rather than have > > > > type=null. > > > > > > >> > > > > > > > > > > >> > > > > Apart from that it seems reasonable to me. Thanks > again, > > > > > > >> > > > > > > > > > > >> > > > > Tom > > > > > > >> > > > > > > > > > > >> > > > > On Thu, Apr 15, 2021 at 3:49 PM Cyrus Vafadari < > > > > > > >> cvafad...@gmail.com> > > > > > > >> > > > > wrote: > > > > > > >> > > > > > > > > > > >> > > > > > Hi Tom, > > > > > > >> > > > > > > > > > > > >> > > > > > Thanks for taking the time to respond with > thoughtful > > > > > > questions: > > > > > > >> > > > > > > > > > > > >> > > > > > 1. I propose HTTP-400, will update the KIP to > reflect > > > this > > > > > > >> proposal > > > > > > >> > > > > > 2. I think the schema should be of the same format. > It > > > is > > > > > > quite > > > > > > >> > > > minimal, > > > > > > >> > > > > so > > > > > > >> > > > > > there is room to add fields in the future without > > > breaking > > > > > > >> > > > compatibility. > > > > > > >> > > > > > I will update the KIP to specify. > > > > > > >> > > > > > 3. Yes, that's right. > > > > > > >> > > > > > 4. I personally don't see the value in deprecating > > since > > > > > it's > > > > > > a > > > > > > >> > > simple > > > > > > >> > > > > > alias (interface and impl will both be simple > > changes). > > > I > > > > > > would > > > > > > >> be > > > > > > >> > > > > > comfortable kicking that can down the road if/when > > there > > > > is > > > > > a > > > > > > >> need > > > > > > >> > > for > > > > > > >> > > > an > > > > > > >> > > > > > actual breaking change. That way we can keep the > scope > > > and > > > > > > diff > > > > > > >> > here > > > > > > >> > > > > tight. > > > > > > >> > > > > > But iff the community feels strongly that > deprecating > > is > > > > the > > > > > > >> right > > > > > > >> > > > thing > > > > > > >> > > > > to > > > > > > >> > > > > > do, I'm happy to update the KIP to propose > > deprecating. > > > > > > >> > > > > > > > > > > > >> > > > > > Thanks again! > > > > > > >> > > > > > > > > > > > >> > > > > > On Wed, Apr 14, 2021 at 9:38 AM Tom Bentley < > > > > > > >> tbent...@redhat.com> > > > > > > >> > > > wrote: > > > > > > >> > > > > > > > > > > > >> > > > > > > Hi Cyrus, > > > > > > >> > > > > > > > > > > > > >> > > > > > > Thanks for the KIP. A few questions: > > > > > > >> > > > > > > > > > > > > >> > > > > > > 1. What status code does the > /plugins/{plugin-type} > > > > > endpoint > > > > > > >> > return > > > > > > >> > > > > when > > > > > > >> > > > > > an > > > > > > >> > > > > > > unknown type is given? > > > > > > >> > > > > > > 2. The result of /connector-plugins is a list of > > > objects > > > > > > with > > > > > > >> > > > 'class', > > > > > > >> > > > > > > 'type' and 'version' properties. Presumably > > > > > > >> /plugins/connector is > > > > > > >> > > the > > > > > > >> > > > > > same, > > > > > > >> > > > > > > but what is the schema for the other plugin types? > > > > > > >> > > > > > > 3. You're not proposing an equivalent of the > > > > > > >> > > > > > > /connector-plugins/{connectorType}/config/validate > > > > > endpoint > > > > > > >> for > > > > > > >> > > > > > > non-connector types, which I think makes sense, > > > because > > > > > you > > > > > > >> would > > > > > > >> > > > > > validate > > > > > > >> > > > > > > an SMT's config via its used by a connector, so > the > > > > > existing > > > > > > >> > > endpoint > > > > > > >> > > > > > > suffices, right? > > > > > > >> > > > > > > 4. Would /connector-plugins eventually be > deprecated > > > in > > > > > > >> favour of > > > > > > >> > > > > > > /plugins/connector, or do we expect it to remain > in > > > the > > > > > API > > > > > > >> > > > > indefinitely > > > > > > >> > > > > > > and accept that /connector-plugins and > > > > /plugins/connector > > > > > > >> provide > > > > > > >> > > > > > identical > > > > > > >> > > > > > > responses? If we had the intention to deprecate in > > the > > > > > > future > > > > > > >> > maybe > > > > > > >> > > > we > > > > > > >> > > > > > > should add a /plugins/connector/config/validate > > > endpoint > > > > > > now? > > > > > > >> > > > > > > > > > > > > >> > > > > > > Many thanks, > > > > > > >> > > > > > > > > > > > > >> > > > > > > Tom > > > > > > >> > > > > > > > > > > > > >> > > > > > > On Tue, Apr 13, 2021 at 6:46 PM Cyrus Vafadari < > > > > > > >> > > cvafad...@gmail.com> > > > > > > >> > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > >> > > > > > > > Hello all, > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > As the number of connect plugins, SMT's, etc > have > > > > > grown, I > > > > > > >> > wanted > > > > > > >> > > > to > > > > > > >> > > > > > bump > > > > > > >> > > > > > > > this thread to see if there is more interest in > > > > adding a > > > > > > >> > Connect > > > > > > >> > > > REST > > > > > > >> > > > > > > > endpoint to get the current plugins in the > worker. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > Thanks all, and thanks Chris for the initial > > > feedback > > > > on > > > > > > >> this. > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > On Mon, Feb 17, 2020 at 12:56 PM Cyrus Vafadari > < > > > > > > >> > > > cvafad...@gmail.com > > > > > > >> > > > > > > > > > > > >> > > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > Thanks for the feedback, Chris. > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > WRT Worker-plugins, I see your point that they > > > > aren't > > > > > > very > > > > > > >> > > useful > > > > > > >> > > > > for > > > > > > >> > > > > > > > > people trying to create connectors to see what > > > > exists > > > > > > >> > already, > > > > > > >> > > > but > > > > > > >> > > > > I > > > > > > >> > > > > > > > would > > > > > > >> > > > > > > > > find them useful for things like making > > assertions > > > > > > >> against a > > > > > > >> > > > docker > > > > > > >> > > > > > > image > > > > > > >> > > > > > > > > to confirm the pluginpaths/classpaths are > > > configured > > > > > > >> > correctly. > > > > > > >> > > > It > > > > > > >> > > > > is > > > > > > >> > > > > > > > more > > > > > > >> > > > > > > > > useful for "sanity check" kinds of things than > > > > > > >> > > > exploring/browsing. > > > > > > >> > > > > > That > > > > > > >> > > > > > > > > said, I don't feel too strongly and if more > > > feedback > > > > > > from > > > > > > >> the > > > > > > >> > > > > > community > > > > > > >> > > > > > > > > thinks they are better excluded then we can > > remove > > > > > them. > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > WRT camelCase vs hyphens, I see your point > here, > > > > will > > > > > > >> update > > > > > > >> > > the > > > > > > >> > > > > KIP. > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > WRT compatibility -- Good question -- I would > > > expect > > > > > > that > > > > > > >> > yes, > > > > > > >> > > > new > > > > > > >> > > > > > > > plugins > > > > > > >> > > > > > > > > would match this format. I will add this > > > explicitly > > > > in > > > > > > the > > > > > > >> > KIP > > > > > > >> > > > for > > > > > > >> > > > > > > > clarity. > > > > > > >> > > > > > > > > I do think it's a valuable, simple feature to > > have > > > > the > > > > > > >> worker > > > > > > >> > > > able > > > > > > >> > > > > to > > > > > > >> > > > > > > > > report new plugins. > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > Thanks again! > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > On Wed, Feb 12, 2020 at 4:44 PM Christopher > > > Egerton > > > > < > > > > > > >> > > > > > > chr...@confluent.io > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > wrote: > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> Hi Cyrus, > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> Thanks for the KIP! > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> One quick question--I see the use case for > > being > > > > able > > > > > > to > > > > > > >> > list > > > > > > >> > > > > > > > >> per-connector > > > > > > >> > > > > > > > >> plugins (SMTs, converters, and header > > converters) > > > > via > > > > > > >> REST > > > > > > >> > > API, > > > > > > >> > > > > but > > > > > > >> > > > > > > I'm > > > > > > >> > > > > > > > >> not > > > > > > >> > > > > > > > >> so sure about worker plugins (REST extensions > > and > > > > > > config > > > > > > >> > > > > providers). > > > > > > >> > > > > > > > Since > > > > > > >> > > > > > > > >> those are configured at startup for the > worker, > > > is > > > > > > there > > > > > > >> any > > > > > > >> > > > > > advantage > > > > > > >> > > > > > > > to > > > > > > >> > > > > > > > >> being able to see the worker plugins > available > > > on a > > > > > > >> running > > > > > > >> > > > > worker? > > > > > > >> > > > > > > It's > > > > > > >> > > > > > > > >> not like they can be added during the > lifetime > > of > > > > > that > > > > > > >> > worker, > > > > > > >> > > > and > > > > > > >> > > > > > > that > > > > > > >> > > > > > > > >> information wouldn't be useful to anyone > trying > > > to > > > > > > >> create a > > > > > > >> > > > > > connector > > > > > > >> > > > > > > as > > > > > > >> > > > > > > > >> they wouldn't be able to include worker > plugins > > > in > > > > a > > > > > > >> > connector > > > > > > >> > > > > > config > > > > > > >> > > > > > > > >> anyways. > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> And a small nit--it seems like the precedent > > set > > > > > > >> > > > ever-so-slightly > > > > > > >> > > > > by > > > > > > >> > > > > > > the > > > > > > >> > > > > > > > >> /connector-plugins resource is to use a > hyphen > > to > > > > > > >> separate a > > > > > > >> > > > > series > > > > > > >> > > > > > of > > > > > > >> > > > > > > > >> lowercase words in an endpoint path as > opposed > > to > > > > > camel > > > > > > >> case > > > > > > >> > > > with > > > > > > >> > > > > > > > >> capitalization. What do you think about > > changing > > > > the > > > > > > >> > possible > > > > > > >> > > > > plugin > > > > > > >> > > > > > > > types > > > > > > >> > > > > > > > >> to "connector", "converter", > > "header-converter", > > > > > etc.? > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> One final question about forwards > > > > compatibility--the > > > > > > >> > > description > > > > > > >> > > > > for > > > > > > >> > > > > > > the > > > > > > >> > > > > > > > >> /plugins endpoint indicates that it "Returns > > > plugin > > > > > > >> > > descriptions > > > > > > >> > > > > of > > > > > > >> > > > > > > all > > > > > > >> > > > > > > > >> types". If another type of pluggable > component > > is > > > > > added > > > > > > >> to > > > > > > >> > the > > > > > > >> > > > > > > > framework, > > > > > > >> > > > > > > > >> should we expect this endpoint to be updated > to > > > > > include > > > > > > >> that > > > > > > >> > > > type > > > > > > >> > > > > of > > > > > > >> > > > > > > > >> plugin > > > > > > >> > > > > > > > >> as well? And if so, should we also expect a > > > > matching > > > > > > >> > > > > > > > /plugins/{pluginType} > > > > > > >> > > > > > > > >> endpoint to be added? > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> Cheers, > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> Chris > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> On Fri, Sep 6, 2019 at 11:48 AM Cyrus > Vafadari > > < > > > > > > >> > > > > cy...@confluent.io> > > > > > > >> > > > > > > > >> wrote: > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > >> > I've updated the KIP here to simplify and > > > clarify > > > > > the > > > > > > >> goal > > > > > > >> > > -- > > > > > > >> > > > > > it's a > > > > > > >> > > > > > > > >> pretty > > > > > > >> > > > > > > > >> > simple KIP to add a REST endpoint for SMTs > > and > > > > > other > > > > > > >> > > plugins. > > > > > > >> > > > > This > > > > > > >> > > > > > > > will > > > > > > >> > > > > > > > >> > make it easier to see what SMTs you've > > loaded. > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > Hoping for some good discussion! > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > Thanks, all > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > On Sat, Jul 20, 2019 at 9:07 PM Cyrus > > Vafadari > > > < > > > > > > >> > > > > > cy...@confluent.io> > > > > > > >> > > > > > > > >> wrote: > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > Hello, all, > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > I'd like to start discussion on a new KIP > > I'm > > > > > > >> proposing > > > > > > >> > to > > > > > > >> > > > add > > > > > > >> > > > > > > HTTP > > > > > > >> > > > > > > > >> > > endpoints to Kafka Connect to give us > some > > > more > > > > > > >> insight > > > > > > >> > > into > > > > > > >> > > > > > other > > > > > > >> > > > > > > > >> > > non-connector plugins, like Simple > Message > > > > > > >> > Transformations > > > > > > >> > > > > > (SMTs). > > > > > > >> > > > > > > > The > > > > > > >> > > > > > > > >> > KIP > > > > > > >> > > > > > > > >> > > would not change how Connect works in any > > > > > > meaningful > > > > > > >> way > > > > > > >> > > > > except > > > > > > >> > > > > > to > > > > > > >> > > > > > > > >> expose > > > > > > >> > > > > > > > >> > > some more data by the endpoint, analogous > > to > > > > > > existing > > > > > > >> > > > > > > > >> > > "ConnectorPluginsResource" class. > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > Looking forward to getting some feedback. > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > Cyrus > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-494%3A+Connect+REST+Endpoint+for+Transformations+%28SMTs%29+and+other+Plugins > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > >