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