[DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi,

We have posted KIP-285: Connect Rest Extension Plugin to add the ability to
provide Rest Extensions to Connect Rest API.

https://cwiki.apache.org/confluence/display/KAFKA/KIP-285%3A+Connect+Rest+Extension+Plugin

Please take a look. Your feedback is appreciated.

Thanks,

Magesh

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi Jason,

Thanks a lot for your feedback. The health interface does provide the same
information that's already exposed in the Connect Rest endpoints. Where it
would be useful is if we need to infer certain kind of health status based
on the connect and task status. One good example would be Liveliness probe
for kubernetes. Depending on how critical the connector is one could decide
the liveliness based on various task status. This is something very
specific to SLA requirements for the connector for a user. Hence exposing
this to Extension does allow users to implement healthchecks based on their
requirements. Let me know your thoughts.

Thanks
Magesh

On Fri, May 18, 2018 at 4:05 PM, Jason Gustafson <ja...@confluent.io> wrote:

> Hi Magesh,
>
> Thanks for the KIP. It's definitely useful to have a pluggable auth layer,
> as we have for the kafka brokers. I was a bit unclear why we needed to
> expose all this health information in the context though. Since it is the
> bigger part of the API, I was hoping to see a little more motivation for
> why a plugin would need this. Do the current status endpoints not integrate
> well with common alerting systems? Is that something that can be fixed?
>
> Thanks,
> Jason
>
> On Thu, May 17, 2018 at 10:05 PM, Ewen Cheslack-Postava <ewen@confluent.io
> >
> wrote:
>
> > Yup, thanks for the changes. The 'health' package in particular feels
> like
> > a nice fit given the way we expect it to be used.
> >
> > -Ewen
> >
> > On Wed, May 16, 2018 at 7:02 PM Randall Hauch <rh...@gmail.com> wrote:
> >
> > > Looks good to me. Thanks for quickly making the changes! Great work!
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > > On May 16, 2018, at 8:07 PM, Magesh Nandakumar <mageshn@confluent.io
> >
> > > wrote:
> > > >
> > > > Randall,
> > > >
> > > > I have adjusted the package names per Ewen's suggestions and also
> made
> > > some
> > > > minor edits per your suggestions. Since there are no major
> outstanding
> > > > issues, i'm moving this to vote.
> > > >
> > > > Thanks
> > > > Magesh
> > > >
> > > >> On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rh...@gmail.com>
> > > wrote:
> > > >>
> > > >> A few very minor suggestions:
> > > >>
> > > >>
> > > >>   1. There are a few formatting issues with paragraphs that use a
> > > >>   monospace font. Minor, but it would be nice to fix.
> > > >>   2. Would be nice to link to the PR
> > > >>   3. Do we need the org.apache.kafka.connect.rest.
> extension.entities
> > > >>   package? Could we just move the two classes into the parent
> > > >>   org.apache.kafka.connect.rest.extension package?
> > > >>   4. This sentence "The above approach helps alleviate any issues
> that
> > > >>   could arise if Extension accidentally reregister the" is cut off.
> > > >>   5. The "ConnectRestExtensionContext.configure(...)" method's
> > JavaDoc
> > > >>   should describe the behaviors that are mentioned in the "Rest
> > > Extension
> > > >>   Integration with Connect" section; e.g., behavior when an
> extension
> > > >> adds a
> > > >>   resource that is already registered, whether unregistering works,
> > etc.
> > > >>   Also, ideally the "close()" method would have JavaDoc that
> explained
> > > >> when
> > > >>   it is called (e.g., no other methods will be called on the
> extension
> > > >> after
> > > >>   this, etc.).
> > > >>   6. Packaging requirements are different for this component vs
> > > >>   connectors, transformations, and converters, since this now
> mandates
> > > the
> > > >>   Service Loader manifest file. This should be called out more
> > > explicitly.
> > > >>   7. It'd be nice if the example included how extension-specific
> > config
> > > >>   properties are to be defined in the worker configuration file.
> > > >>
> > > >> As I said, these are all minor suggestions that only affect the KIP
> > > >> document. Once these are fixed, I think this is ready to move to
> > voting.
> > > >>
> > > >> Best regards,
> > > >>
> > > >> Randall
> > > >>
> > > >> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <
> > > mageshn@confluent.io>
> > > >> wrote:
> > > >>
> > > >>> Randall- I think I have addressed all the comments. Let me know if
> we
> > > can
> > > >>> take this to Vote.
> > > >>>
> > > >>> Thanks
> > > >>> Magesh
> > > >>>
> > > >>> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <
> > > mageshn@confluent.io
> > > >>>
> > > >>> wrote:
> > > >>>
> > > >>>> Hi All,
> > > >>>>
> > > >>>> I have updated the KIP to reflect changes based on the PR
> > > >>>> https://github.com/apache/kafka/pull/4931. Its mostly has minor
> > > >> changes
> > > >>>> to the interfaces and includes details on packages for the
> > interfaces
> > > >> and
> > > >>>> the classes. Let me know your thoughts.
> > > >>>>
> > > >>>> Thanks
> > > >>>> Magesh
> > > >>>>
> > > >>>> On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rhauch@gmail.com
> >
> > > >>> wrote:
> > > >>>>
> > > >>>>> Great work, Magesh. I like the overall approach a lot, so I left
> > some
> > > >>>>> pretty nuanced comments about specific details.
> > > >>>>>
> > > >>>>> Best regards,
> > > >>>>>
> > > >>>>> Randall
> > > >>>>>
> > > >>>>> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> > > >>> mageshn@confluent.io>
> > > >>>>> wrote:
> > > >>>>>
> > > >>>>>> Thanks Randall for your thoughts. I have created a replica of
> the
> > > >>>>> required
> > > >>>>>> entities in the draft implementation. If you can take a look at
> > the
> > > >> PR
> > > >>>>> and
> > > >>>>>> let me know your thoughts, I will update the KIP to reflect the
> > same
> > > >>>>>>
> > > >>>>>> https://github.com/apache/kafka/pull/4931
> > > >>>>>>
> > > >>>>>> On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <
> rhauch@gmail.com
> > >
> > > >>>>> wrote:
> > > >>>>>>
> > > >>>>>>> Magesh, I think our last emails cross in mid-stream.
> > > >>>>>>>
> > > >>>>>>> We definitely want to put the new public interfaces/classes in
> > the
> > > >>> API
> > > >>>>>>> module, and implementation in the runtime module. Yes, this
> will
> > > >>>>> affect
> > > >>>>>> the
> > > >>>>>>> design, since for example we don't want to expose runtime types
> > to
> > > >>> the
> > > >>>>>> API,
> > > >>>>>>> and we want to prevent breaking changes. We don't really want
> to
> > > >>> move
> > > >>>>> the
> > > >>>>>>> REST entities if we don't have to, since that may break
> projects
> > > >>> that
> > > >>>>> are
> > > >>>>>>> extending the runtime module -- even though the runtime module
> is
> > > >>> not
> > > >>>>> a
> > > >>>>>>> public API we still want to _try_ to change things.
> > > >>>>>>>
> > > >>>>>>> Do you want to try to create a prototype to see what kind of
> > > >> impact
> > > >>>>> and
> > > >>>>>>> choices we'll have to make?
> > > >>>>>>>
> > > >>>>>>> Best regards,
> > > >>>>>>>
> > > >>>>>>> Randall
> > > >>>>>>>
> > > >>>>>>> On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <
> > rhauch@gmail.com
> > > >>>
> > > >>>>>> wrote:
> > > >>>>>>>
> > > >>>>>>>> Thanks for updating the KIP, Magesh. You've resolved all of my
> > > >>>>>> concerns,
> > > >>>>>>>> though I have one more: we should specify the package names
> for
> > > >>> all
> > > >>>>> new
> > > >>>>>>>> interfaces/classes.
> > > >>>>>>>>
> > > >>>>>>>> I'm looking forward to more feedback from others.
> > > >>>>>>>>
> > > >>>>>>>> Best regards,
> > > >>>>>>>>
> > > >>>>>>>> Randall
> > > >>>>>>>>
> > > >>>>>>>> On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > > >>>>>>> mageshn@confluent.io>
> > > >>>>>>>> wrote:
> > > >>>>>>>>
> > > >>>>>>>>> Hi All,
> > > >>>>>>>>>
> > > >>>>>>>>> I have updated the KIP with following changes
> > > >>>>>>>>>
> > > >>>>>>>>>   1. Expanded the Motivation section
> > > >>>>>>>>>   2. Included details about the interface in the public
> > > >>> interface
> > > >>>>>>> section
> > > >>>>>>>>>   3. Modified the config name to rest.extension.classes
> > > >>>>>>>>>   4. Modified the ConnectRestExtension to include
> Configurable
> > > >>>>>> instead
> > > >>>>>>> of
> > > >>>>>>>>>   ResourceConfig
> > > >>>>>>>>>   5. Modified the "Rest Extension Integration with Connect"
> in
> > > >>>>>>> "Proposed
> > > >>>>>>>>>   Approach" to include a new Custom implementation for
> > > >>>>> Configurable
> > > >>>>>>>>>   6. Provided examples for the Java Service provider
> mechanism
> > > >>>>>>>>>   7. Included a reference implementation in scope
> > > >>>>>>>>>
> > > >>>>>>>>> Kindly let me know your thoughts on the updates.
> > > >>>>>>>>>
> > > >>>>>>>>> Thanks
> > > >>>>>>>>> Magesh
> > > >>>>>>>>>
> > > >>>>>>>>> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > > >>>>>>> mageshn@confluent.io
> > > >>>>>>>>>>
> > > >>>>>>>>> wrote:
> > > >>>>>>>>>
> > > >>>>>>>>>> Hi Randall,
> > > >>>>>>>>>>
> > > >>>>>>>>>> Thanks for your feedback. I also would like to go with
> > > >>>>>>>>>> rest.extension.classes`.
> > > >>>>>>>>>>
> > > >>>>>>>>>> For exposing Configurable, my original intention was just to
> > > >>>>> expose
> > > >>>>>>> that
> > > >>>>>>>>>> to the extension because that's all one needs to register
> JAX
> > > >>> RS
> > > >>>>>>>>> resources.
> > > >>>>>>>>>> The fact that we use Jersey shouldn't even be exposed in the
> > > >>>>>>> interface.
> > > >>>>>>>>>> Hence it doesn't affect the public API by any means.
> > > >>>>>>>>>>
> > > >>>>>>>>>> I will update the KIP and let everyone know.
> > > >>>>>>>>>>
> > > >>>>>>>>>> Thanks
> > > >>>>>>>>>> Magesh
> > > >>>>>>>>>>
> > > >>>>>>>>>> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> > > >>> rhauch@gmail.com
> > > >>>>>>
> > > >>>>>>>>> wrote:
> > > >>>>>>>>>>
> > > >>>>>>>>>>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > > >>>>>>>>> mageshn@confluent.io
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>> wrote:
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>> Hi Randall,
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Thanks a lot for your feedback.
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> I will update the KIP to reflect your comments in (1),
> > > >> (2),
> > > >>>>> (7)
> > > >>>>>> and
> > > >>>>>>>>> (8).
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Looking forward to these.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> For comment # (3) , while it would be great to
> > > >> automatically
> > > >>>>>>>>> configure
> > > >>>>>>>>>>> the
> > > >>>>>>>>>>>> Rest Extensions, I would prefer that to be specified
> > > >>>>> explicitly.
> > > >>>>>>> Lets
> > > >>>>>>>>>>>> assume a connector archive includes a implementation for
> > > >> the
> > > >>>>>>>>>>> RestExtension
> > > >>>>>>>>>>>> to do authentication using some header. We don't want this
> > > >>> to
> > > >>>>> be
> > > >>>>>>>>>>>> automatically included. Having said that I think that the
> > > >>>>> config
> > > >>>>>>> key
> > > >>>>>>>>>>> name
> > > >>>>>>>>>>>> should probably be changed to something like
> > > >>> "rest.extension"
> > > >>>>> or
> > > >>>>>>>>>>>> "rest.extension.class".
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> That's a good point. I do like `rest.extension.class` (or
> > > >>>>>>> `..classes`?)
> > > >>>>>>>>>>> much more than `rest.plugins`.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> For the comment regarding the resource loading into
> > > >> jersey,
> > > >>> I
> > > >>>>>> have
> > > >>>>>>>>> the
> > > >>>>>>>>>>>> following proposal
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Create an implementation of Configurable(
> > > >>>>>>>>>>>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> > > >>> Config
> > > >>>>>>>>> urable.html
> > > >>>>>>>>>>> )
> > > >>>>>>>>>>>> that delegates to ResourceConfig. In the
> > > >>>>> ConnectRestExtension, we
> > > >>>>>>>>> would
> > > >>>>>>>>>>>> expose only Configurable which is sufficient enough to
> > > >>>>> register
> > > >>>>>> new
> > > >>>>>>>>>>>> resources. In the new implementation, we will check if the
> > > >>>>>> resource
> > > >>>>>>>>> is
> > > >>>>>>>>>>>> already registered using ResourceConfig.isRegistered()
> > > >>> method
> > > >>>>> and
> > > >>>>>>>>> log a
> > > >>>>>>>>>>>> warning if the resource is already registered. This will
> > > >>> make
> > > >>>>> it
> > > >>>>>> a
> > > >>>>>>>>>>>> deterministic behavior and avoid any potential
> > > >>>>> re-registrations.
> > > >>>>>>> Let
> > > >>>>>>>>> me
> > > >>>>>>>>>>>> know your thoughts on these.
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> This sounds a good idea. Is it as flexible as the current
> > > >>>>> proposal?
> > > >>>>>>> If
> > > >>>>>>>>>>> not,
> > > >>>>>>>>>>> then I'd love to see how this affects the public APIs.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Thanks
> > > >>>>>>>>>>>> Magesh
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> > > >>>>>> rhauch@gmail.com>
> > > >>>>>>>>>>> wrote:
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>>> Very nice proposal, Magesh. I like the approach and the
> > > >>> new
> > > >>>>>>>>> concepts
> > > >>>>>>>>>>> and
> > > >>>>>>>>>>>>> interfaces, but I do have a few comments/suggestions
> > > >> about
> > > >>>>> some
> > > >>>>>>>>>>> specific
> > > >>>>>>>>>>>>> details:
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>>   1. In the "Motivation" section, perhaps it makes
> > > >> sense
> > > >>> to
> > > >>>>>>>>> briefly
> > > >>>>>>>>>>>>>   describe one or two somewhat concrete examples of how
> > > >>>>> this
> > > >>>>>> is
> > > >>>>>>>>>>> useful.
> > > >>>>>>>>>>>>>   2. Maybe in the "Public Interfaces" section you could
> > > >>>>>> briefly
> > > >>>>>>>>>>> describe
> > > >>>>>>>>>>>>>   each of the interfaces, what they represent, and
> > > >> which
> > > >>>>> are
> > > >>>>>>>>>>> implemented
> > > >>>>>>>>>>>>> by
> > > >>>>>>>>>>>>>   the framework vs implemented by an extension. I think
> > > >>>>> it'd
> > > >>>>>>> help
> > > >>>>>>>>> to
> > > >>>>>>>>>>>>> explain
> > > >>>>>>>>>>>>>   that only the `ConnectRestPlugin` needs to be
> > > >>>>> implemented,
> > > >>>>>> and
> > > >>>>>>>>> the
> > > >>>>>>>>>>>> rest
> > > >>>>>>>>>>>>>   will be provided by the framework. I know the next
> > > >>>>> section
> > > >>>>>>> goes
> > > >>>>>>>>>>> into
> > > >>>>>>>>>>>> it
> > > >>>>>>>>>>>>> a
> > > >>>>>>>>>>>>>   bit, but it'd be useful in this section when first
> > > >>>>> talking
> > > >>>>>>> about
> > > >>>>>>>>>>> the
> > > >>>>>>>>>>>> new
> > > >>>>>>>>>>>>>   interfaces.
> > > >>>>>>>>>>>>>   3. Also in the "Public Interfaces" section: I don't
> > > >>>>> think we
> > > >>>>>>>>> should
> > > >>>>>>>>>>>>>   introduce a "rest.plugins" configuration property.
> > > >>>>> Instead,
> > > >>>>>>> can
> > > >>>>>>>>> we
> > > >>>>>>>>>>> not
> > > >>>>>>>>>>>>> just
> > > >>>>>>>>>>>>>   instantiate and call all of the ConnectRestPlugins
> > > >> that
> > > >>>>> we
> > > >>>>>>> find
> > > >>>>>>>>> on
> > > >>>>>>>>>>> the
> > > >>>>>>>>>>>>>   plugin path? Besides, it seems too close to the
> > > >>>>>> `plugin.path`
> > > >>>>>>>>>>>>> configuration
> > > >>>>>>>>>>>>>   property.
> > > >>>>>>>>>>>>>   4. Why would the implementation register Connect
> > > >>>>> resources
> > > >>>>>>>>> *after*
> > > >>>>>>>>>>> the
> > > >>>>>>>>>>>>>   plugins, if Jersey currently registers only the first
> > > >>>>> one?
> > > >>>>>> The
> > > >>>>>>>>>>>> "Rejected
> > > >>>>>>>>>>>>>   Alternatives" mentions why, but this section should
> > > >> be
> > > >>>>>>> explicit
> > > >>>>>>>>>>> about
> > > >>>>>>>>>>>>> why.
> > > >>>>>>>>>>>>>   For example, "The plugin's would be registered in the
> > > >>>>>>>>>>>>>   RestServer.start(Herder herder) method before
> > > >>> registering
> > > >>>>>> the
> > > >>>>>>>>>>> default
> > > >>>>>>>>>>>>>   Connect resources, which ensures that plugins cannot
> > > >>>>> remove
> > > >>>>>>>>> Connect
> > > >>>>>>>>>>>>>   resources."
> > > >>>>>>>>>>>>>   5. "Hence, it is recommended that the plugins don't
> > > >>>>>>> re-register
> > > >>>>>>>>> the
> > > >>>>>>>>>>>>>   default Connect Resources. This could potentially
> > > >> lead
> > > >>> to
> > > >>>>>>>>>>> unexpected
> > > >>>>>>>>>>>>>   errors." First, we should not say "recommended" and
> > > >>>>> should
> > > >>>>>>> just
> > > >>>>>>>>> say
> > > >>>>>>>>>>>>> plugins
> > > >>>>>>>>>>>>>   should not register any resources that conflict with
> > > >>> the
> > > >>>>>>>>> built-in
> > > >>>>>>>>>>>>> Connect
> > > >>>>>>>>>>>>>   resources. Second, if the worker does find conflicts,
> > > >>>>> can we
> > > >>>>>>>>> just
> > > >>>>>>>>>>>> remove
> > > >>>>>>>>>>>>>   them before adding the built-in Connect resources?
> > > >>>>>>>>>>>>>   6. Is it possible for implementations to check
> > > >> whether
> > > >>>>>>> resources
> > > >>>>>>>>>>>> already
> > > >>>>>>>>>>>>>   exist before registering their own? If so, we should
> > > >>>>>> recommend
> > > >>>>>>>>> that
> > > >>>>>>>>>>>>>   implementations do this and log any problems.
> > > >>>>>>>>>>>>>   7. We should be explicit that the "Service Provider"
> > > >> is
> > > >>>>>> Java's
> > > >>>>>>>>>>> Service
> > > >>>>>>>>>>>>>   Provider API. We also need to be explicit that an
> > > >>>>>>> implementation
> > > >>>>>>>>>>> must
> > > >>>>>>>>>>>>>   provide a `META-INF/services/org.apache.
> > > >> kafka.connect.
> > > >>>>>>>>>>>>> ConnectRestPlugin`
> > > >>>>>>>>>>>>>   file (or whatever the package name of the
> > > >>>>>> `ConnectRestPlugin`
> > > >>>>>>>>> will
> > > >>>>>>>>>>> be)
> > > >>>>>>>>>>>>> with
> > > >>>>>>>>>>>>>   the fully-qualified name of the implementation
> > > >>> class(es).
> > > >>>>>>>>>>>>>   8. The example should include the META-INF file
> > > >>> required
> > > >>>>> by
> > > >>>>>>> the
> > > >>>>>>>>>>>> Service
> > > >>>>>>>>>>>>>   Provider API.
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>> Again, overall this is really great!
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>> Best regards,
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>> Randall
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>> On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > > >>>>>>>>>>>> mageshn@confluent.io>
> > > >>>>>>>>>>>>> wrote:
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> Hi,
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> We have posted KIP-285: Connect Rest Extension Plugin
> > > >> to
> > > >>>>> add
> > > >>>>>>> the
> > > >>>>>>>>>>>> ability
> > > >>>>>>>>>>>>> to
> > > >>>>>>>>>>>>>> provide Rest Extensions to Connect Rest API.
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> https://cwiki.apache.org/
> > > >> confluence/display/KAFKA/KIP-
> > > >>>>>>>>>>>>>> 285%3A+Connect+Rest+Extension+Plugin
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> Please take a look. Your feedback is appreciated.
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> Thanks,
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>> Magesh
> > > >>>>>>>>>>>>>>
> > > >>>>>>>>>>>>>
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Jason Gustafson <ja...@confluent.io>]

Hi Magesh,

Thanks for the KIP. It's definitely useful to have a pluggable auth layer,
as we have for the kafka brokers. I was a bit unclear why we needed to
expose all this health information in the context though. Since it is the
bigger part of the API, I was hoping to see a little more motivation for
why a plugin would need this. Do the current status endpoints not integrate
well with common alerting systems? Is that something that can be fixed?

Thanks,
Jason

On Thu, May 17, 2018 at 10:05 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:

> Yup, thanks for the changes. The 'health' package in particular feels like
> a nice fit given the way we expect it to be used.
>
> -Ewen
>
> On Wed, May 16, 2018 at 7:02 PM Randall Hauch <rh...@gmail.com> wrote:
>
> > Looks good to me. Thanks for quickly making the changes! Great work!
> >
> > Best regards,
> >
> > Randall
> >
> > > On May 16, 2018, at 8:07 PM, Magesh Nandakumar <ma...@confluent.io>
> > wrote:
> > >
> > > Randall,
> > >
> > > I have adjusted the package names per Ewen's suggestions and also made
> > some
> > > minor edits per your suggestions. Since there are no major outstanding
> > > issues, i'm moving this to vote.
> > >
> > > Thanks
> > > Magesh
> > >
> > >> On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rh...@gmail.com>
> > wrote:
> > >>
> > >> A few very minor suggestions:
> > >>
> > >>
> > >>   1. There are a few formatting issues with paragraphs that use a
> > >>   monospace font. Minor, but it would be nice to fix.
> > >>   2. Would be nice to link to the PR
> > >>   3. Do we need the org.apache.kafka.connect.rest.extension.entities
> > >>   package? Could we just move the two classes into the parent
> > >>   org.apache.kafka.connect.rest.extension package?
> > >>   4. This sentence "The above approach helps alleviate any issues that
> > >>   could arise if Extension accidentally reregister the" is cut off.
> > >>   5. The "ConnectRestExtensionContext.configure(...)" method's
> JavaDoc
> > >>   should describe the behaviors that are mentioned in the "Rest
> > Extension
> > >>   Integration with Connect" section; e.g., behavior when an extension
> > >> adds a
> > >>   resource that is already registered, whether unregistering works,
> etc.
> > >>   Also, ideally the "close()" method would have JavaDoc that explained
> > >> when
> > >>   it is called (e.g., no other methods will be called on the extension
> > >> after
> > >>   this, etc.).
> > >>   6. Packaging requirements are different for this component vs
> > >>   connectors, transformations, and converters, since this now mandates
> > the
> > >>   Service Loader manifest file. This should be called out more
> > explicitly.
> > >>   7. It'd be nice if the example included how extension-specific
> config
> > >>   properties are to be defined in the worker configuration file.
> > >>
> > >> As I said, these are all minor suggestions that only affect the KIP
> > >> document. Once these are fixed, I think this is ready to move to
> voting.
> > >>
> > >> Best regards,
> > >>
> > >> Randall
> > >>
> > >> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > >> wrote:
> > >>
> > >>> Randall- I think I have addressed all the comments. Let me know if we
> > can
> > >>> take this to Vote.
> > >>>
> > >>> Thanks
> > >>> Magesh
> > >>>
> > >>> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <
> > mageshn@confluent.io
> > >>>
> > >>> wrote:
> > >>>
> > >>>> Hi All,
> > >>>>
> > >>>> I have updated the KIP to reflect changes based on the PR
> > >>>> https://github.com/apache/kafka/pull/4931. Its mostly has minor
> > >> changes
> > >>>> to the interfaces and includes details on packages for the
> interfaces
> > >> and
> > >>>> the classes. Let me know your thoughts.
> > >>>>
> > >>>> Thanks
> > >>>> Magesh
> > >>>>
> > >>>> On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> > >>> wrote:
> > >>>>
> > >>>>> Great work, Magesh. I like the overall approach a lot, so I left
> some
> > >>>>> pretty nuanced comments about specific details.
> > >>>>>
> > >>>>> Best regards,
> > >>>>>
> > >>>>> Randall
> > >>>>>
> > >>>>> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> > >>> mageshn@confluent.io>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> Thanks Randall for your thoughts. I have created a replica of the
> > >>>>> required
> > >>>>>> entities in the draft implementation. If you can take a look at
> the
> > >> PR
> > >>>>> and
> > >>>>>> let me know your thoughts, I will update the KIP to reflect the
> same
> > >>>>>>
> > >>>>>> https://github.com/apache/kafka/pull/4931
> > >>>>>>
> > >>>>>> On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rhauch@gmail.com
> >
> > >>>>> wrote:
> > >>>>>>
> > >>>>>>> Magesh, I think our last emails cross in mid-stream.
> > >>>>>>>
> > >>>>>>> We definitely want to put the new public interfaces/classes in
> the
> > >>> API
> > >>>>>>> module, and implementation in the runtime module. Yes, this will
> > >>>>> affect
> > >>>>>> the
> > >>>>>>> design, since for example we don't want to expose runtime types
> to
> > >>> the
> > >>>>>> API,
> > >>>>>>> and we want to prevent breaking changes. We don't really want to
> > >>> move
> > >>>>> the
> > >>>>>>> REST entities if we don't have to, since that may break projects
> > >>> that
> > >>>>> are
> > >>>>>>> extending the runtime module -- even though the runtime module is
> > >>> not
> > >>>>> a
> > >>>>>>> public API we still want to _try_ to change things.
> > >>>>>>>
> > >>>>>>> Do you want to try to create a prototype to see what kind of
> > >> impact
> > >>>>> and
> > >>>>>>> choices we'll have to make?
> > >>>>>>>
> > >>>>>>> Best regards,
> > >>>>>>>
> > >>>>>>> Randall
> > >>>>>>>
> > >>>>>>> On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <
> rhauch@gmail.com
> > >>>
> > >>>>>> wrote:
> > >>>>>>>
> > >>>>>>>> Thanks for updating the KIP, Magesh. You've resolved all of my
> > >>>>>> concerns,
> > >>>>>>>> though I have one more: we should specify the package names for
> > >>> all
> > >>>>> new
> > >>>>>>>> interfaces/classes.
> > >>>>>>>>
> > >>>>>>>> I'm looking forward to more feedback from others.
> > >>>>>>>>
> > >>>>>>>> Best regards,
> > >>>>>>>>
> > >>>>>>>> Randall
> > >>>>>>>>
> > >>>>>>>> On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > >>>>>>> mageshn@confluent.io>
> > >>>>>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> Hi All,
> > >>>>>>>>>
> > >>>>>>>>> I have updated the KIP with following changes
> > >>>>>>>>>
> > >>>>>>>>>   1. Expanded the Motivation section
> > >>>>>>>>>   2. Included details about the interface in the public
> > >>> interface
> > >>>>>>> section
> > >>>>>>>>>   3. Modified the config name to rest.extension.classes
> > >>>>>>>>>   4. Modified the ConnectRestExtension to include Configurable
> > >>>>>> instead
> > >>>>>>> of
> > >>>>>>>>>   ResourceConfig
> > >>>>>>>>>   5. Modified the "Rest Extension Integration with Connect" in
> > >>>>>>> "Proposed
> > >>>>>>>>>   Approach" to include a new Custom implementation for
> > >>>>> Configurable
> > >>>>>>>>>   6. Provided examples for the Java Service provider mechanism
> > >>>>>>>>>   7. Included a reference implementation in scope
> > >>>>>>>>>
> > >>>>>>>>> Kindly let me know your thoughts on the updates.
> > >>>>>>>>>
> > >>>>>>>>> Thanks
> > >>>>>>>>> Magesh
> > >>>>>>>>>
> > >>>>>>>>> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > >>>>>>> mageshn@confluent.io
> > >>>>>>>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> Hi Randall,
> > >>>>>>>>>>
> > >>>>>>>>>> Thanks for your feedback. I also would like to go with
> > >>>>>>>>>> rest.extension.classes`.
> > >>>>>>>>>>
> > >>>>>>>>>> For exposing Configurable, my original intention was just to
> > >>>>> expose
> > >>>>>>> that
> > >>>>>>>>>> to the extension because that's all one needs to register JAX
> > >>> RS
> > >>>>>>>>> resources.
> > >>>>>>>>>> The fact that we use Jersey shouldn't even be exposed in the
> > >>>>>>> interface.
> > >>>>>>>>>> Hence it doesn't affect the public API by any means.
> > >>>>>>>>>>
> > >>>>>>>>>> I will update the KIP and let everyone know.
> > >>>>>>>>>>
> > >>>>>>>>>> Thanks
> > >>>>>>>>>> Magesh
> > >>>>>>>>>>
> > >>>>>>>>>> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> > >>> rhauch@gmail.com
> > >>>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>>
> > >>>>>>>>>>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > >>>>>>>>> mageshn@confluent.io
> > >>>>>>>>>>>>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> Hi Randall,
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Thanks a lot for your feedback.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> I will update the KIP to reflect your comments in (1),
> > >> (2),
> > >>>>> (7)
> > >>>>>> and
> > >>>>>>>>> (8).
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> Looking forward to these.
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> For comment # (3) , while it would be great to
> > >> automatically
> > >>>>>>>>> configure
> > >>>>>>>>>>> the
> > >>>>>>>>>>>> Rest Extensions, I would prefer that to be specified
> > >>>>> explicitly.
> > >>>>>>> Lets
> > >>>>>>>>>>>> assume a connector archive includes a implementation for
> > >> the
> > >>>>>>>>>>> RestExtension
> > >>>>>>>>>>>> to do authentication using some header. We don't want this
> > >>> to
> > >>>>> be
> > >>>>>>>>>>>> automatically included. Having said that I think that the
> > >>>>> config
> > >>>>>>> key
> > >>>>>>>>>>> name
> > >>>>>>>>>>>> should probably be changed to something like
> > >>> "rest.extension"
> > >>>>> or
> > >>>>>>>>>>>> "rest.extension.class".
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> That's a good point. I do like `rest.extension.class` (or
> > >>>>>>> `..classes`?)
> > >>>>>>>>>>> much more than `rest.plugins`.
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> For the comment regarding the resource loading into
> > >> jersey,
> > >>> I
> > >>>>>> have
> > >>>>>>>>> the
> > >>>>>>>>>>>> following proposal
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Create an implementation of Configurable(
> > >>>>>>>>>>>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> > >>> Config
> > >>>>>>>>> urable.html
> > >>>>>>>>>>> )
> > >>>>>>>>>>>> that delegates to ResourceConfig. In the
> > >>>>> ConnectRestExtension, we
> > >>>>>>>>> would
> > >>>>>>>>>>>> expose only Configurable which is sufficient enough to
> > >>>>> register
> > >>>>>> new
> > >>>>>>>>>>>> resources. In the new implementation, we will check if the
> > >>>>>> resource
> > >>>>>>>>> is
> > >>>>>>>>>>>> already registered using ResourceConfig.isRegistered()
> > >>> method
> > >>>>> and
> > >>>>>>>>> log a
> > >>>>>>>>>>>> warning if the resource is already registered. This will
> > >>> make
> > >>>>> it
> > >>>>>> a
> > >>>>>>>>>>>> deterministic behavior and avoid any potential
> > >>>>> re-registrations.
> > >>>>>>> Let
> > >>>>>>>>> me
> > >>>>>>>>>>>> know your thoughts on these.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> This sounds a good idea. Is it as flexible as the current
> > >>>>> proposal?
> > >>>>>>> If
> > >>>>>>>>>>> not,
> > >>>>>>>>>>> then I'd love to see how this affects the public APIs.
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Thanks
> > >>>>>>>>>>>> Magesh
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> > >>>>>> rhauch@gmail.com>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>> Very nice proposal, Magesh. I like the approach and the
> > >>> new
> > >>>>>>>>> concepts
> > >>>>>>>>>>> and
> > >>>>>>>>>>>>> interfaces, but I do have a few comments/suggestions
> > >> about
> > >>>>> some
> > >>>>>>>>>>> specific
> > >>>>>>>>>>>>> details:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>   1. In the "Motivation" section, perhaps it makes
> > >> sense
> > >>> to
> > >>>>>>>>> briefly
> > >>>>>>>>>>>>>   describe one or two somewhat concrete examples of how
> > >>>>> this
> > >>>>>> is
> > >>>>>>>>>>> useful.
> > >>>>>>>>>>>>>   2. Maybe in the "Public Interfaces" section you could
> > >>>>>> briefly
> > >>>>>>>>>>> describe
> > >>>>>>>>>>>>>   each of the interfaces, what they represent, and
> > >> which
> > >>>>> are
> > >>>>>>>>>>> implemented
> > >>>>>>>>>>>>> by
> > >>>>>>>>>>>>>   the framework vs implemented by an extension. I think
> > >>>>> it'd
> > >>>>>>> help
> > >>>>>>>>> to
> > >>>>>>>>>>>>> explain
> > >>>>>>>>>>>>>   that only the `ConnectRestPlugin` needs to be
> > >>>>> implemented,
> > >>>>>> and
> > >>>>>>>>> the
> > >>>>>>>>>>>> rest
> > >>>>>>>>>>>>>   will be provided by the framework. I know the next
> > >>>>> section
> > >>>>>>> goes
> > >>>>>>>>>>> into
> > >>>>>>>>>>>> it
> > >>>>>>>>>>>>> a
> > >>>>>>>>>>>>>   bit, but it'd be useful in this section when first
> > >>>>> talking
> > >>>>>>> about
> > >>>>>>>>>>> the
> > >>>>>>>>>>>> new
> > >>>>>>>>>>>>>   interfaces.
> > >>>>>>>>>>>>>   3. Also in the "Public Interfaces" section: I don't
> > >>>>> think we
> > >>>>>>>>> should
> > >>>>>>>>>>>>>   introduce a "rest.plugins" configuration property.
> > >>>>> Instead,
> > >>>>>>> can
> > >>>>>>>>> we
> > >>>>>>>>>>> not
> > >>>>>>>>>>>>> just
> > >>>>>>>>>>>>>   instantiate and call all of the ConnectRestPlugins
> > >> that
> > >>>>> we
> > >>>>>>> find
> > >>>>>>>>> on
> > >>>>>>>>>>> the
> > >>>>>>>>>>>>>   plugin path? Besides, it seems too close to the
> > >>>>>> `plugin.path`
> > >>>>>>>>>>>>> configuration
> > >>>>>>>>>>>>>   property.
> > >>>>>>>>>>>>>   4. Why would the implementation register Connect
> > >>>>> resources
> > >>>>>>>>> *after*
> > >>>>>>>>>>> the
> > >>>>>>>>>>>>>   plugins, if Jersey currently registers only the first
> > >>>>> one?
> > >>>>>> The
> > >>>>>>>>>>>> "Rejected
> > >>>>>>>>>>>>>   Alternatives" mentions why, but this section should
> > >> be
> > >>>>>>> explicit
> > >>>>>>>>>>> about
> > >>>>>>>>>>>>> why.
> > >>>>>>>>>>>>>   For example, "The plugin's would be registered in the
> > >>>>>>>>>>>>>   RestServer.start(Herder herder) method before
> > >>> registering
> > >>>>>> the
> > >>>>>>>>>>> default
> > >>>>>>>>>>>>>   Connect resources, which ensures that plugins cannot
> > >>>>> remove
> > >>>>>>>>> Connect
> > >>>>>>>>>>>>>   resources."
> > >>>>>>>>>>>>>   5. "Hence, it is recommended that the plugins don't
> > >>>>>>> re-register
> > >>>>>>>>> the
> > >>>>>>>>>>>>>   default Connect Resources. This could potentially
> > >> lead
> > >>> to
> > >>>>>>>>>>> unexpected
> > >>>>>>>>>>>>>   errors." First, we should not say "recommended" and
> > >>>>> should
> > >>>>>>> just
> > >>>>>>>>> say
> > >>>>>>>>>>>>> plugins
> > >>>>>>>>>>>>>   should not register any resources that conflict with
> > >>> the
> > >>>>>>>>> built-in
> > >>>>>>>>>>>>> Connect
> > >>>>>>>>>>>>>   resources. Second, if the worker does find conflicts,
> > >>>>> can we
> > >>>>>>>>> just
> > >>>>>>>>>>>> remove
> > >>>>>>>>>>>>>   them before adding the built-in Connect resources?
> > >>>>>>>>>>>>>   6. Is it possible for implementations to check
> > >> whether
> > >>>>>>> resources
> > >>>>>>>>>>>> already
> > >>>>>>>>>>>>>   exist before registering their own? If so, we should
> > >>>>>> recommend
> > >>>>>>>>> that
> > >>>>>>>>>>>>>   implementations do this and log any problems.
> > >>>>>>>>>>>>>   7. We should be explicit that the "Service Provider"
> > >> is
> > >>>>>> Java's
> > >>>>>>>>>>> Service
> > >>>>>>>>>>>>>   Provider API. We also need to be explicit that an
> > >>>>>>> implementation
> > >>>>>>>>>>> must
> > >>>>>>>>>>>>>   provide a `META-INF/services/org.apache.
> > >> kafka.connect.
> > >>>>>>>>>>>>> ConnectRestPlugin`
> > >>>>>>>>>>>>>   file (or whatever the package name of the
> > >>>>>> `ConnectRestPlugin`
> > >>>>>>>>> will
> > >>>>>>>>>>> be)
> > >>>>>>>>>>>>> with
> > >>>>>>>>>>>>>   the fully-qualified name of the implementation
> > >>> class(es).
> > >>>>>>>>>>>>>   8. The example should include the META-INF file
> > >>> required
> > >>>>> by
> > >>>>>>> the
> > >>>>>>>>>>>> Service
> > >>>>>>>>>>>>>   Provider API.
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Again, overall this is really great!
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Best regards,
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Randall
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > >>>>>>>>>>>> mageshn@confluent.io>
> > >>>>>>>>>>>>> wrote:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Hi,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> We have posted KIP-285: Connect Rest Extension Plugin
> > >> to
> > >>>>> add
> > >>>>>>> the
> > >>>>>>>>>>>> ability
> > >>>>>>>>>>>>> to
> > >>>>>>>>>>>>>> provide Rest Extensions to Connect Rest API.
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> https://cwiki.apache.org/
> > >> confluence/display/KAFKA/KIP-
> > >>>>>>>>>>>>>> 285%3A+Connect+Rest+Extension+Plugin
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Please take a look. Your feedback is appreciated.
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Thanks,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> Magesh
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>
> > >>
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Ewen Cheslack-Postava <ew...@confluent.io>]

Yup, thanks for the changes. The 'health' package in particular feels like
a nice fit given the way we expect it to be used.

-Ewen

On Wed, May 16, 2018 at 7:02 PM Randall Hauch <rh...@gmail.com> wrote:

> Looks good to me. Thanks for quickly making the changes! Great work!
>
> Best regards,
>
> Randall
>
> > On May 16, 2018, at 8:07 PM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
> >
> > Randall,
> >
> > I have adjusted the package names per Ewen's suggestions and also made
> some
> > minor edits per your suggestions. Since there are no major outstanding
> > issues, i'm moving this to vote.
> >
> > Thanks
> > Magesh
> >
> >> On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rh...@gmail.com>
> wrote:
> >>
> >> A few very minor suggestions:
> >>
> >>
> >>   1. There are a few formatting issues with paragraphs that use a
> >>   monospace font. Minor, but it would be nice to fix.
> >>   2. Would be nice to link to the PR
> >>   3. Do we need the org.apache.kafka.connect.rest.extension.entities
> >>   package? Could we just move the two classes into the parent
> >>   org.apache.kafka.connect.rest.extension package?
> >>   4. This sentence "The above approach helps alleviate any issues that
> >>   could arise if Extension accidentally reregister the" is cut off.
> >>   5. The "ConnectRestExtensionContext.configure(...)" method's JavaDoc
> >>   should describe the behaviors that are mentioned in the "Rest
> Extension
> >>   Integration with Connect" section; e.g., behavior when an extension
> >> adds a
> >>   resource that is already registered, whether unregistering works, etc.
> >>   Also, ideally the "close()" method would have JavaDoc that explained
> >> when
> >>   it is called (e.g., no other methods will be called on the extension
> >> after
> >>   this, etc.).
> >>   6. Packaging requirements are different for this component vs
> >>   connectors, transformations, and converters, since this now mandates
> the
> >>   Service Loader manifest file. This should be called out more
> explicitly.
> >>   7. It'd be nice if the example included how extension-specific config
> >>   properties are to be defined in the worker configuration file.
> >>
> >> As I said, these are all minor suggestions that only affect the KIP
> >> document. Once these are fixed, I think this is ready to move to voting.
> >>
> >> Best regards,
> >>
> >> Randall
> >>
> >> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <
> mageshn@confluent.io>
> >> wrote:
> >>
> >>> Randall- I think I have addressed all the comments. Let me know if we
> can
> >>> take this to Vote.
> >>>
> >>> Thanks
> >>> Magesh
> >>>
> >>> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <
> mageshn@confluent.io
> >>>
> >>> wrote:
> >>>
> >>>> Hi All,
> >>>>
> >>>> I have updated the KIP to reflect changes based on the PR
> >>>> https://github.com/apache/kafka/pull/4931. Its mostly has minor
> >> changes
> >>>> to the interfaces and includes details on packages for the interfaces
> >> and
> >>>> the classes. Let me know your thoughts.
> >>>>
> >>>> Thanks
> >>>> Magesh
> >>>>
> >>>> On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> >>> wrote:
> >>>>
> >>>>> Great work, Magesh. I like the overall approach a lot, so I left some
> >>>>> pretty nuanced comments about specific details.
> >>>>>
> >>>>> Best regards,
> >>>>>
> >>>>> Randall
> >>>>>
> >>>>> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> >>> mageshn@confluent.io>
> >>>>> wrote:
> >>>>>
> >>>>>> Thanks Randall for your thoughts. I have created a replica of the
> >>>>> required
> >>>>>> entities in the draft implementation. If you can take a look at the
> >> PR
> >>>>> and
> >>>>>> let me know your thoughts, I will update the KIP to reflect the same
> >>>>>>
> >>>>>> https://github.com/apache/kafka/pull/4931
> >>>>>>
> >>>>>> On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> >>>>> wrote:
> >>>>>>
> >>>>>>> Magesh, I think our last emails cross in mid-stream.
> >>>>>>>
> >>>>>>> We definitely want to put the new public interfaces/classes in the
> >>> API
> >>>>>>> module, and implementation in the runtime module. Yes, this will
> >>>>> affect
> >>>>>> the
> >>>>>>> design, since for example we don't want to expose runtime types to
> >>> the
> >>>>>> API,
> >>>>>>> and we want to prevent breaking changes. We don't really want to
> >>> move
> >>>>> the
> >>>>>>> REST entities if we don't have to, since that may break projects
> >>> that
> >>>>> are
> >>>>>>> extending the runtime module -- even though the runtime module is
> >>> not
> >>>>> a
> >>>>>>> public API we still want to _try_ to change things.
> >>>>>>>
> >>>>>>> Do you want to try to create a prototype to see what kind of
> >> impact
> >>>>> and
> >>>>>>> choices we'll have to make?
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>>
> >>>>>>> Randall
> >>>>>>>
> >>>>>>> On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rhauch@gmail.com
> >>>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>>> Thanks for updating the KIP, Magesh. You've resolved all of my
> >>>>>> concerns,
> >>>>>>>> though I have one more: we should specify the package names for
> >>> all
> >>>>> new
> >>>>>>>> interfaces/classes.
> >>>>>>>>
> >>>>>>>> I'm looking forward to more feedback from others.
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>>
> >>>>>>>> Randall
> >>>>>>>>
> >>>>>>>> On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> >>>>>>> mageshn@confluent.io>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Hi All,
> >>>>>>>>>
> >>>>>>>>> I have updated the KIP with following changes
> >>>>>>>>>
> >>>>>>>>>   1. Expanded the Motivation section
> >>>>>>>>>   2. Included details about the interface in the public
> >>> interface
> >>>>>>> section
> >>>>>>>>>   3. Modified the config name to rest.extension.classes
> >>>>>>>>>   4. Modified the ConnectRestExtension to include Configurable
> >>>>>> instead
> >>>>>>> of
> >>>>>>>>>   ResourceConfig
> >>>>>>>>>   5. Modified the "Rest Extension Integration with Connect" in
> >>>>>>> "Proposed
> >>>>>>>>>   Approach" to include a new Custom implementation for
> >>>>> Configurable
> >>>>>>>>>   6. Provided examples for the Java Service provider mechanism
> >>>>>>>>>   7. Included a reference implementation in scope
> >>>>>>>>>
> >>>>>>>>> Kindly let me know your thoughts on the updates.
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Magesh
> >>>>>>>>>
> >>>>>>>>> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> >>>>>>> mageshn@confluent.io
> >>>>>>>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> Hi Randall,
> >>>>>>>>>>
> >>>>>>>>>> Thanks for your feedback. I also would like to go with
> >>>>>>>>>> rest.extension.classes`.
> >>>>>>>>>>
> >>>>>>>>>> For exposing Configurable, my original intention was just to
> >>>>> expose
> >>>>>>> that
> >>>>>>>>>> to the extension because that's all one needs to register JAX
> >>> RS
> >>>>>>>>> resources.
> >>>>>>>>>> The fact that we use Jersey shouldn't even be exposed in the
> >>>>>>> interface.
> >>>>>>>>>> Hence it doesn't affect the public API by any means.
> >>>>>>>>>>
> >>>>>>>>>> I will update the KIP and let everyone know.
> >>>>>>>>>>
> >>>>>>>>>> Thanks
> >>>>>>>>>> Magesh
> >>>>>>>>>>
> >>>>>>>>>> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> >>> rhauch@gmail.com
> >>>>>>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> >>>>>>>>> mageshn@confluent.io
> >>>>>>>>>>>>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Hi Randall,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks a lot for your feedback.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I will update the KIP to reflect your comments in (1),
> >> (2),
> >>>>> (7)
> >>>>>> and
> >>>>>>>>> (8).
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Looking forward to these.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> For comment # (3) , while it would be great to
> >> automatically
> >>>>>>>>> configure
> >>>>>>>>>>> the
> >>>>>>>>>>>> Rest Extensions, I would prefer that to be specified
> >>>>> explicitly.
> >>>>>>> Lets
> >>>>>>>>>>>> assume a connector archive includes a implementation for
> >> the
> >>>>>>>>>>> RestExtension
> >>>>>>>>>>>> to do authentication using some header. We don't want this
> >>> to
> >>>>> be
> >>>>>>>>>>>> automatically included. Having said that I think that the
> >>>>> config
> >>>>>>> key
> >>>>>>>>>>> name
> >>>>>>>>>>>> should probably be changed to something like
> >>> "rest.extension"
> >>>>> or
> >>>>>>>>>>>> "rest.extension.class".
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> That's a good point. I do like `rest.extension.class` (or
> >>>>>>> `..classes`?)
> >>>>>>>>>>> much more than `rest.plugins`.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> For the comment regarding the resource loading into
> >> jersey,
> >>> I
> >>>>>> have
> >>>>>>>>> the
> >>>>>>>>>>>> following proposal
> >>>>>>>>>>>>
> >>>>>>>>>>>> Create an implementation of Configurable(
> >>>>>>>>>>>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> >>> Config
> >>>>>>>>> urable.html
> >>>>>>>>>>> )
> >>>>>>>>>>>> that delegates to ResourceConfig. In the
> >>>>> ConnectRestExtension, we
> >>>>>>>>> would
> >>>>>>>>>>>> expose only Configurable which is sufficient enough to
> >>>>> register
> >>>>>> new
> >>>>>>>>>>>> resources. In the new implementation, we will check if the
> >>>>>> resource
> >>>>>>>>> is
> >>>>>>>>>>>> already registered using ResourceConfig.isRegistered()
> >>> method
> >>>>> and
> >>>>>>>>> log a
> >>>>>>>>>>>> warning if the resource is already registered. This will
> >>> make
> >>>>> it
> >>>>>> a
> >>>>>>>>>>>> deterministic behavior and avoid any potential
> >>>>> re-registrations.
> >>>>>>> Let
> >>>>>>>>> me
> >>>>>>>>>>>> know your thoughts on these.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> This sounds a good idea. Is it as flexible as the current
> >>>>> proposal?
> >>>>>>> If
> >>>>>>>>>>> not,
> >>>>>>>>>>> then I'd love to see how this affects the public APIs.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks
> >>>>>>>>>>>> Magesh
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> >>>>>> rhauch@gmail.com>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Very nice proposal, Magesh. I like the approach and the
> >>> new
> >>>>>>>>> concepts
> >>>>>>>>>>> and
> >>>>>>>>>>>>> interfaces, but I do have a few comments/suggestions
> >> about
> >>>>> some
> >>>>>>>>>>> specific
> >>>>>>>>>>>>> details:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>   1. In the "Motivation" section, perhaps it makes
> >> sense
> >>> to
> >>>>>>>>> briefly
> >>>>>>>>>>>>>   describe one or two somewhat concrete examples of how
> >>>>> this
> >>>>>> is
> >>>>>>>>>>> useful.
> >>>>>>>>>>>>>   2. Maybe in the "Public Interfaces" section you could
> >>>>>> briefly
> >>>>>>>>>>> describe
> >>>>>>>>>>>>>   each of the interfaces, what they represent, and
> >> which
> >>>>> are
> >>>>>>>>>>> implemented
> >>>>>>>>>>>>> by
> >>>>>>>>>>>>>   the framework vs implemented by an extension. I think
> >>>>> it'd
> >>>>>>> help
> >>>>>>>>> to
> >>>>>>>>>>>>> explain
> >>>>>>>>>>>>>   that only the `ConnectRestPlugin` needs to be
> >>>>> implemented,
> >>>>>> and
> >>>>>>>>> the
> >>>>>>>>>>>> rest
> >>>>>>>>>>>>>   will be provided by the framework. I know the next
> >>>>> section
> >>>>>>> goes
> >>>>>>>>>>> into
> >>>>>>>>>>>> it
> >>>>>>>>>>>>> a
> >>>>>>>>>>>>>   bit, but it'd be useful in this section when first
> >>>>> talking
> >>>>>>> about
> >>>>>>>>>>> the
> >>>>>>>>>>>> new
> >>>>>>>>>>>>>   interfaces.
> >>>>>>>>>>>>>   3. Also in the "Public Interfaces" section: I don't
> >>>>> think we
> >>>>>>>>> should
> >>>>>>>>>>>>>   introduce a "rest.plugins" configuration property.
> >>>>> Instead,
> >>>>>>> can
> >>>>>>>>> we
> >>>>>>>>>>> not
> >>>>>>>>>>>>> just
> >>>>>>>>>>>>>   instantiate and call all of the ConnectRestPlugins
> >> that
> >>>>> we
> >>>>>>> find
> >>>>>>>>> on
> >>>>>>>>>>> the
> >>>>>>>>>>>>>   plugin path? Besides, it seems too close to the
> >>>>>> `plugin.path`
> >>>>>>>>>>>>> configuration
> >>>>>>>>>>>>>   property.
> >>>>>>>>>>>>>   4. Why would the implementation register Connect
> >>>>> resources
> >>>>>>>>> *after*
> >>>>>>>>>>> the
> >>>>>>>>>>>>>   plugins, if Jersey currently registers only the first
> >>>>> one?
> >>>>>> The
> >>>>>>>>>>>> "Rejected
> >>>>>>>>>>>>>   Alternatives" mentions why, but this section should
> >> be
> >>>>>>> explicit
> >>>>>>>>>>> about
> >>>>>>>>>>>>> why.
> >>>>>>>>>>>>>   For example, "The plugin's would be registered in the
> >>>>>>>>>>>>>   RestServer.start(Herder herder) method before
> >>> registering
> >>>>>> the
> >>>>>>>>>>> default
> >>>>>>>>>>>>>   Connect resources, which ensures that plugins cannot
> >>>>> remove
> >>>>>>>>> Connect
> >>>>>>>>>>>>>   resources."
> >>>>>>>>>>>>>   5. "Hence, it is recommended that the plugins don't
> >>>>>>> re-register
> >>>>>>>>> the
> >>>>>>>>>>>>>   default Connect Resources. This could potentially
> >> lead
> >>> to
> >>>>>>>>>>> unexpected
> >>>>>>>>>>>>>   errors." First, we should not say "recommended" and
> >>>>> should
> >>>>>>> just
> >>>>>>>>> say
> >>>>>>>>>>>>> plugins
> >>>>>>>>>>>>>   should not register any resources that conflict with
> >>> the
> >>>>>>>>> built-in
> >>>>>>>>>>>>> Connect
> >>>>>>>>>>>>>   resources. Second, if the worker does find conflicts,
> >>>>> can we
> >>>>>>>>> just
> >>>>>>>>>>>> remove
> >>>>>>>>>>>>>   them before adding the built-in Connect resources?
> >>>>>>>>>>>>>   6. Is it possible for implementations to check
> >> whether
> >>>>>>> resources
> >>>>>>>>>>>> already
> >>>>>>>>>>>>>   exist before registering their own? If so, we should
> >>>>>> recommend
> >>>>>>>>> that
> >>>>>>>>>>>>>   implementations do this and log any problems.
> >>>>>>>>>>>>>   7. We should be explicit that the "Service Provider"
> >> is
> >>>>>> Java's
> >>>>>>>>>>> Service
> >>>>>>>>>>>>>   Provider API. We also need to be explicit that an
> >>>>>>> implementation
> >>>>>>>>>>> must
> >>>>>>>>>>>>>   provide a `META-INF/services/org.apache.
> >> kafka.connect.
> >>>>>>>>>>>>> ConnectRestPlugin`
> >>>>>>>>>>>>>   file (or whatever the package name of the
> >>>>>> `ConnectRestPlugin`
> >>>>>>>>> will
> >>>>>>>>>>> be)
> >>>>>>>>>>>>> with
> >>>>>>>>>>>>>   the fully-qualified name of the implementation
> >>> class(es).
> >>>>>>>>>>>>>   8. The example should include the META-INF file
> >>> required
> >>>>> by
> >>>>>>> the
> >>>>>>>>>>>> Service
> >>>>>>>>>>>>>   Provider API.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Again, overall this is really great!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Best regards,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Randall
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> >>>>>>>>>>>> mageshn@confluent.io>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> We have posted KIP-285: Connect Rest Extension Plugin
> >> to
> >>>>> add
> >>>>>>> the
> >>>>>>>>>>>> ability
> >>>>>>>>>>>>> to
> >>>>>>>>>>>>>> provide Rest Extensions to Connect Rest API.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://cwiki.apache.org/
> >> confluence/display/KAFKA/KIP-
> >>>>>>>>>>>>>> 285%3A+Connect+Rest+Extension+Plugin
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Please take a look. Your feedback is appreciated.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Magesh
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

Looks good to me. Thanks for quickly making the changes! Great work!
 
Best regards,

Randall

> On May 16, 2018, at 8:07 PM, Magesh Nandakumar <ma...@confluent.io> wrote:
> 
> Randall,
> 
> I have adjusted the package names per Ewen's suggestions and also made some
> minor edits per your suggestions. Since there are no major outstanding
> issues, i'm moving this to vote.
> 
> Thanks
> Magesh
> 
>> On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rh...@gmail.com> wrote:
>> 
>> A few very minor suggestions:
>> 
>> 
>>   1. There are a few formatting issues with paragraphs that use a
>>   monospace font. Minor, but it would be nice to fix.
>>   2. Would be nice to link to the PR
>>   3. Do we need the org.apache.kafka.connect.rest.extension.entities
>>   package? Could we just move the two classes into the parent
>>   org.apache.kafka.connect.rest.extension package?
>>   4. This sentence "The above approach helps alleviate any issues that
>>   could arise if Extension accidentally reregister the" is cut off.
>>   5. The "ConnectRestExtensionContext.configure(...)" method's JavaDoc
>>   should describe the behaviors that are mentioned in the "Rest Extension
>>   Integration with Connect" section; e.g., behavior when an extension
>> adds a
>>   resource that is already registered, whether unregistering works, etc.
>>   Also, ideally the "close()" method would have JavaDoc that explained
>> when
>>   it is called (e.g., no other methods will be called on the extension
>> after
>>   this, etc.).
>>   6. Packaging requirements are different for this component vs
>>   connectors, transformations, and converters, since this now mandates the
>>   Service Loader manifest file. This should be called out more explicitly.
>>   7. It'd be nice if the example included how extension-specific config
>>   properties are to be defined in the worker configuration file.
>> 
>> As I said, these are all minor suggestions that only affect the KIP
>> document. Once these are fixed, I think this is ready to move to voting.
>> 
>> Best regards,
>> 
>> Randall
>> 
>> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <ma...@confluent.io>
>> wrote:
>> 
>>> Randall- I think I have addressed all the comments. Let me know if we can
>>> take this to Vote.
>>> 
>>> Thanks
>>> Magesh
>>> 
>>> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <mageshn@confluent.io
>>> 
>>> wrote:
>>> 
>>>> Hi All,
>>>> 
>>>> I have updated the KIP to reflect changes based on the PR
>>>> https://github.com/apache/kafka/pull/4931. Its mostly has minor
>> changes
>>>> to the interfaces and includes details on packages for the interfaces
>> and
>>>> the classes. Let me know your thoughts.
>>>> 
>>>> Thanks
>>>> Magesh
>>>> 
>>>> On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
>>> wrote:
>>>> 
>>>>> Great work, Magesh. I like the overall approach a lot, so I left some
>>>>> pretty nuanced comments about specific details.
>>>>> 
>>>>> Best regards,
>>>>> 
>>>>> Randall
>>>>> 
>>>>> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
>>> mageshn@confluent.io>
>>>>> wrote:
>>>>> 
>>>>>> Thanks Randall for your thoughts. I have created a replica of the
>>>>> required
>>>>>> entities in the draft implementation. If you can take a look at the
>> PR
>>>>> and
>>>>>> let me know your thoughts, I will update the KIP to reflect the same
>>>>>> 
>>>>>> https://github.com/apache/kafka/pull/4931
>>>>>> 
>>>>>> On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
>>>>> wrote:
>>>>>> 
>>>>>>> Magesh, I think our last emails cross in mid-stream.
>>>>>>> 
>>>>>>> We definitely want to put the new public interfaces/classes in the
>>> API
>>>>>>> module, and implementation in the runtime module. Yes, this will
>>>>> affect
>>>>>> the
>>>>>>> design, since for example we don't want to expose runtime types to
>>> the
>>>>>> API,
>>>>>>> and we want to prevent breaking changes. We don't really want to
>>> move
>>>>> the
>>>>>>> REST entities if we don't have to, since that may break projects
>>> that
>>>>> are
>>>>>>> extending the runtime module -- even though the runtime module is
>>> not
>>>>> a
>>>>>>> public API we still want to _try_ to change things.
>>>>>>> 
>>>>>>> Do you want to try to create a prototype to see what kind of
>> impact
>>>>> and
>>>>>>> choices we'll have to make?
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> 
>>>>>>> Randall
>>>>>>> 
>>>>>>> On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rhauch@gmail.com
>>> 
>>>>>> wrote:
>>>>>>> 
>>>>>>>> Thanks for updating the KIP, Magesh. You've resolved all of my
>>>>>> concerns,
>>>>>>>> though I have one more: we should specify the package names for
>>> all
>>>>> new
>>>>>>>> interfaces/classes.
>>>>>>>> 
>>>>>>>> I'm looking forward to more feedback from others.
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> 
>>>>>>>> Randall
>>>>>>>> 
>>>>>>>> On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
>>>>>>> mageshn@confluent.io>
>>>>>>>> wrote:
>>>>>>>> 
>>>>>>>>> Hi All,
>>>>>>>>> 
>>>>>>>>> I have updated the KIP with following changes
>>>>>>>>> 
>>>>>>>>>   1. Expanded the Motivation section
>>>>>>>>>   2. Included details about the interface in the public
>>> interface
>>>>>>> section
>>>>>>>>>   3. Modified the config name to rest.extension.classes
>>>>>>>>>   4. Modified the ConnectRestExtension to include Configurable
>>>>>> instead
>>>>>>> of
>>>>>>>>>   ResourceConfig
>>>>>>>>>   5. Modified the "Rest Extension Integration with Connect" in
>>>>>>> "Proposed
>>>>>>>>>   Approach" to include a new Custom implementation for
>>>>> Configurable
>>>>>>>>>   6. Provided examples for the Java Service provider mechanism
>>>>>>>>>   7. Included a reference implementation in scope
>>>>>>>>> 
>>>>>>>>> Kindly let me know your thoughts on the updates.
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> Magesh
>>>>>>>>> 
>>>>>>>>> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
>>>>>>> mageshn@confluent.io
>>>>>>>>>> 
>>>>>>>>> wrote:
>>>>>>>>> 
>>>>>>>>>> Hi Randall,
>>>>>>>>>> 
>>>>>>>>>> Thanks for your feedback. I also would like to go with
>>>>>>>>>> rest.extension.classes`.
>>>>>>>>>> 
>>>>>>>>>> For exposing Configurable, my original intention was just to
>>>>> expose
>>>>>>> that
>>>>>>>>>> to the extension because that's all one needs to register JAX
>>> RS
>>>>>>>>> resources.
>>>>>>>>>> The fact that we use Jersey shouldn't even be exposed in the
>>>>>>> interface.
>>>>>>>>>> Hence it doesn't affect the public API by any means.
>>>>>>>>>> 
>>>>>>>>>> I will update the KIP and let everyone know.
>>>>>>>>>> 
>>>>>>>>>> Thanks
>>>>>>>>>> Magesh
>>>>>>>>>> 
>>>>>>>>>> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
>>> rhauch@gmail.com
>>>>>> 
>>>>>>>>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
>>>>>>>>> mageshn@confluent.io
>>>>>>>>>>>> 
>>>>>>>>>>> wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> Hi Randall,
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks a lot for your feedback.
>>>>>>>>>>>> 
>>>>>>>>>>>> I will update the KIP to reflect your comments in (1),
>> (2),
>>>>> (7)
>>>>>> and
>>>>>>>>> (8).
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Looking forward to these.
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> For comment # (3) , while it would be great to
>> automatically
>>>>>>>>> configure
>>>>>>>>>>> the
>>>>>>>>>>>> Rest Extensions, I would prefer that to be specified
>>>>> explicitly.
>>>>>>> Lets
>>>>>>>>>>>> assume a connector archive includes a implementation for
>> the
>>>>>>>>>>> RestExtension
>>>>>>>>>>>> to do authentication using some header. We don't want this
>>> to
>>>>> be
>>>>>>>>>>>> automatically included. Having said that I think that the
>>>>> config
>>>>>>> key
>>>>>>>>>>> name
>>>>>>>>>>>> should probably be changed to something like
>>> "rest.extension"
>>>>> or
>>>>>>>>>>>> "rest.extension.class".
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> That's a good point. I do like `rest.extension.class` (or
>>>>>>> `..classes`?)
>>>>>>>>>>> much more than `rest.plugins`.
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> For the comment regarding the resource loading into
>> jersey,
>>> I
>>>>>> have
>>>>>>>>> the
>>>>>>>>>>>> following proposal
>>>>>>>>>>>> 
>>>>>>>>>>>> Create an implementation of Configurable(
>>>>>>>>>>>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
>>> Config
>>>>>>>>> urable.html
>>>>>>>>>>> )
>>>>>>>>>>>> that delegates to ResourceConfig. In the
>>>>> ConnectRestExtension, we
>>>>>>>>> would
>>>>>>>>>>>> expose only Configurable which is sufficient enough to
>>>>> register
>>>>>> new
>>>>>>>>>>>> resources. In the new implementation, we will check if the
>>>>>> resource
>>>>>>>>> is
>>>>>>>>>>>> already registered using ResourceConfig.isRegistered()
>>> method
>>>>> and
>>>>>>>>> log a
>>>>>>>>>>>> warning if the resource is already registered. This will
>>> make
>>>>> it
>>>>>> a
>>>>>>>>>>>> deterministic behavior and avoid any potential
>>>>> re-registrations.
>>>>>>> Let
>>>>>>>>> me
>>>>>>>>>>>> know your thoughts on these.
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> This sounds a good idea. Is it as flexible as the current
>>>>> proposal?
>>>>>>> If
>>>>>>>>>>> not,
>>>>>>>>>>> then I'd love to see how this affects the public APIs.
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Magesh
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
>>>>>> rhauch@gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>>> Very nice proposal, Magesh. I like the approach and the
>>> new
>>>>>>>>> concepts
>>>>>>>>>>> and
>>>>>>>>>>>>> interfaces, but I do have a few comments/suggestions
>> about
>>>>> some
>>>>>>>>>>> specific
>>>>>>>>>>>>> details:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>   1. In the "Motivation" section, perhaps it makes
>> sense
>>> to
>>>>>>>>> briefly
>>>>>>>>>>>>>   describe one or two somewhat concrete examples of how
>>>>> this
>>>>>> is
>>>>>>>>>>> useful.
>>>>>>>>>>>>>   2. Maybe in the "Public Interfaces" section you could
>>>>>> briefly
>>>>>>>>>>> describe
>>>>>>>>>>>>>   each of the interfaces, what they represent, and
>> which
>>>>> are
>>>>>>>>>>> implemented
>>>>>>>>>>>>> by
>>>>>>>>>>>>>   the framework vs implemented by an extension. I think
>>>>> it'd
>>>>>>> help
>>>>>>>>> to
>>>>>>>>>>>>> explain
>>>>>>>>>>>>>   that only the `ConnectRestPlugin` needs to be
>>>>> implemented,
>>>>>> and
>>>>>>>>> the
>>>>>>>>>>>> rest
>>>>>>>>>>>>>   will be provided by the framework. I know the next
>>>>> section
>>>>>>> goes
>>>>>>>>>>> into
>>>>>>>>>>>> it
>>>>>>>>>>>>> a
>>>>>>>>>>>>>   bit, but it'd be useful in this section when first
>>>>> talking
>>>>>>> about
>>>>>>>>>>> the
>>>>>>>>>>>> new
>>>>>>>>>>>>>   interfaces.
>>>>>>>>>>>>>   3. Also in the "Public Interfaces" section: I don't
>>>>> think we
>>>>>>>>> should
>>>>>>>>>>>>>   introduce a "rest.plugins" configuration property.
>>>>> Instead,
>>>>>>> can
>>>>>>>>> we
>>>>>>>>>>> not
>>>>>>>>>>>>> just
>>>>>>>>>>>>>   instantiate and call all of the ConnectRestPlugins
>> that
>>>>> we
>>>>>>> find
>>>>>>>>> on
>>>>>>>>>>> the
>>>>>>>>>>>>>   plugin path? Besides, it seems too close to the
>>>>>> `plugin.path`
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>   property.
>>>>>>>>>>>>>   4. Why would the implementation register Connect
>>>>> resources
>>>>>>>>> *after*
>>>>>>>>>>> the
>>>>>>>>>>>>>   plugins, if Jersey currently registers only the first
>>>>> one?
>>>>>> The
>>>>>>>>>>>> "Rejected
>>>>>>>>>>>>>   Alternatives" mentions why, but this section should
>> be
>>>>>>> explicit
>>>>>>>>>>> about
>>>>>>>>>>>>> why.
>>>>>>>>>>>>>   For example, "The plugin's would be registered in the
>>>>>>>>>>>>>   RestServer.start(Herder herder) method before
>>> registering
>>>>>> the
>>>>>>>>>>> default
>>>>>>>>>>>>>   Connect resources, which ensures that plugins cannot
>>>>> remove
>>>>>>>>> Connect
>>>>>>>>>>>>>   resources."
>>>>>>>>>>>>>   5. "Hence, it is recommended that the plugins don't
>>>>>>> re-register
>>>>>>>>> the
>>>>>>>>>>>>>   default Connect Resources. This could potentially
>> lead
>>> to
>>>>>>>>>>> unexpected
>>>>>>>>>>>>>   errors." First, we should not say "recommended" and
>>>>> should
>>>>>>> just
>>>>>>>>> say
>>>>>>>>>>>>> plugins
>>>>>>>>>>>>>   should not register any resources that conflict with
>>> the
>>>>>>>>> built-in
>>>>>>>>>>>>> Connect
>>>>>>>>>>>>>   resources. Second, if the worker does find conflicts,
>>>>> can we
>>>>>>>>> just
>>>>>>>>>>>> remove
>>>>>>>>>>>>>   them before adding the built-in Connect resources?
>>>>>>>>>>>>>   6. Is it possible for implementations to check
>> whether
>>>>>>> resources
>>>>>>>>>>>> already
>>>>>>>>>>>>>   exist before registering their own? If so, we should
>>>>>> recommend
>>>>>>>>> that
>>>>>>>>>>>>>   implementations do this and log any problems.
>>>>>>>>>>>>>   7. We should be explicit that the "Service Provider"
>> is
>>>>>> Java's
>>>>>>>>>>> Service
>>>>>>>>>>>>>   Provider API. We also need to be explicit that an
>>>>>>> implementation
>>>>>>>>>>> must
>>>>>>>>>>>>>   provide a `META-INF/services/org.apache.
>> kafka.connect.
>>>>>>>>>>>>> ConnectRestPlugin`
>>>>>>>>>>>>>   file (or whatever the package name of the
>>>>>> `ConnectRestPlugin`
>>>>>>>>> will
>>>>>>>>>>> be)
>>>>>>>>>>>>> with
>>>>>>>>>>>>>   the fully-qualified name of the implementation
>>> class(es).
>>>>>>>>>>>>>   8. The example should include the META-INF file
>>> required
>>>>> by
>>>>>>> the
>>>>>>>>>>>> Service
>>>>>>>>>>>>>   Provider API.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Again, overall this is really great!
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Best regards,
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Randall
>>>>>>>>>>>>> 
>>>>>>>>>>>>> On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
>>>>>>>>>>>> mageshn@confluent.io>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> We have posted KIP-285: Connect Rest Extension Plugin
>> to
>>>>> add
>>>>>>> the
>>>>>>>>>>>> ability
>>>>>>>>>>>>> to
>>>>>>>>>>>>>> provide Rest Extensions to Connect Rest API.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> https://cwiki.apache.org/
>> confluence/display/KAFKA/KIP-
>>>>>>>>>>>>>> 285%3A+Connect+Rest+Extension+Plugin
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Please take a look. Your feedback is appreciated.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Magesh
>>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>> 

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Randall,

I have adjusted the package names per Ewen's suggestions and also made some
minor edits per your suggestions. Since there are no major outstanding
issues, i'm moving this to vote.

Thanks
Magesh

On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rh...@gmail.com> wrote:

> A few very minor suggestions:
>
>
>    1. There are a few formatting issues with paragraphs that use a
>    monospace font. Minor, but it would be nice to fix.
>    2. Would be nice to link to the PR
>    3. Do we need the org.apache.kafka.connect.rest.extension.entities
>    package? Could we just move the two classes into the parent
>    org.apache.kafka.connect.rest.extension package?
>    4. This sentence "The above approach helps alleviate any issues that
>    could arise if Extension accidentally reregister the" is cut off.
>    5. The "ConnectRestExtensionContext.configure(...)" method's JavaDoc
>    should describe the behaviors that are mentioned in the "Rest Extension
>    Integration with Connect" section; e.g., behavior when an extension
> adds a
>    resource that is already registered, whether unregistering works, etc.
>    Also, ideally the "close()" method would have JavaDoc that explained
> when
>    it is called (e.g., no other methods will be called on the extension
> after
>    this, etc.).
>    6. Packaging requirements are different for this component vs
>    connectors, transformations, and converters, since this now mandates the
>    Service Loader manifest file. This should be called out more explicitly.
>    7. It'd be nice if the example included how extension-specific config
>    properties are to be defined in the worker configuration file.
>
> As I said, these are all minor suggestions that only affect the KIP
> document. Once these are fixed, I think this is ready to move to voting.
>
> Best regards,
>
> Randall
>
> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Randall- I think I have addressed all the comments. Let me know if we can
> > take this to Vote.
> >
> > Thanks
> > Magesh
> >
> > On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <mageshn@confluent.io
> >
> > wrote:
> >
> > > Hi All,
> > >
> > > I have updated the KIP to reflect changes based on the PR
> > > https://github.com/apache/kafka/pull/4931. Its mostly has minor
> changes
> > > to the interfaces and includes details on packages for the interfaces
> and
> > > the classes. Let me know your thoughts.
> > >
> > > Thanks
> > > Magesh
> > >
> > > On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> > wrote:
> > >
> > >> Great work, Magesh. I like the overall approach a lot, so I left some
> > >> pretty nuanced comments about specific details.
> > >>
> > >> Best regards,
> > >>
> > >> Randall
> > >>
> > >> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > >> wrote:
> > >>
> > >> > Thanks Randall for your thoughts. I have created a replica of the
> > >> required
> > >> > entities in the draft implementation. If you can take a look at the
> PR
> > >> and
> > >> > let me know your thoughts, I will update the KIP to reflect the same
> > >> >
> > >> > https://github.com/apache/kafka/pull/4931
> > >> >
> > >> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> > >> wrote:
> > >> >
> > >> > > Magesh, I think our last emails cross in mid-stream.
> > >> > >
> > >> > > We definitely want to put the new public interfaces/classes in the
> > API
> > >> > > module, and implementation in the runtime module. Yes, this will
> > >> affect
> > >> > the
> > >> > > design, since for example we don't want to expose runtime types to
> > the
> > >> > API,
> > >> > > and we want to prevent breaking changes. We don't really want to
> > move
> > >> the
> > >> > > REST entities if we don't have to, since that may break projects
> > that
> > >> are
> > >> > > extending the runtime module -- even though the runtime module is
> > not
> > >> a
> > >> > > public API we still want to _try_ to change things.
> > >> > >
> > >> > > Do you want to try to create a prototype to see what kind of
> impact
> > >> and
> > >> > > choices we'll have to make?
> > >> > >
> > >> > > Best regards,
> > >> > >
> > >> > > Randall
> > >> > >
> > >> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rhauch@gmail.com
> >
> > >> > wrote:
> > >> > >
> > >> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
> > >> > concerns,
> > >> > > > though I have one more: we should specify the package names for
> > all
> > >> new
> > >> > > > interfaces/classes.
> > >> > > >
> > >> > > > I'm looking forward to more feedback from others.
> > >> > > >
> > >> > > > Best regards,
> > >> > > >
> > >> > > > Randall
> > >> > > >
> > >> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > >> > > mageshn@confluent.io>
> > >> > > > wrote:
> > >> > > >
> > >> > > >> Hi All,
> > >> > > >>
> > >> > > >> I have updated the KIP with following changes
> > >> > > >>
> > >> > > >>    1. Expanded the Motivation section
> > >> > > >>    2. Included details about the interface in the public
> > interface
> > >> > > section
> > >> > > >>    3. Modified the config name to rest.extension.classes
> > >> > > >>    4. Modified the ConnectRestExtension to include Configurable
> > >> > instead
> > >> > > of
> > >> > > >>    ResourceConfig
> > >> > > >>    5. Modified the "Rest Extension Integration with Connect" in
> > >> > > "Proposed
> > >> > > >>    Approach" to include a new Custom implementation for
> > >> Configurable
> > >> > > >>    6. Provided examples for the Java Service provider mechanism
> > >> > > >>    7. Included a reference implementation in scope
> > >> > > >>
> > >> > > >> Kindly let me know your thoughts on the updates.
> > >> > > >>
> > >> > > >> Thanks
> > >> > > >> Magesh
> > >> > > >>
> > >> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > >> > > mageshn@confluent.io
> > >> > > >> >
> > >> > > >> wrote:
> > >> > > >>
> > >> > > >> > Hi Randall,
> > >> > > >> >
> > >> > > >> > Thanks for your feedback. I also would like to go with
> > >> > > >> > rest.extension.classes`.
> > >> > > >> >
> > >> > > >> > For exposing Configurable, my original intention was just to
> > >> expose
> > >> > > that
> > >> > > >> > to the extension because that's all one needs to register JAX
> > RS
> > >> > > >> resources.
> > >> > > >> > The fact that we use Jersey shouldn't even be exposed in the
> > >> > > interface.
> > >> > > >> > Hence it doesn't affect the public API by any means.
> > >> > > >> >
> > >> > > >> > I will update the KIP and let everyone know.
> > >> > > >> >
> > >> > > >> > Thanks
> > >> > > >> > Magesh
> > >> > > >> >
> > >> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> > rhauch@gmail.com
> > >> >
> > >> > > >> wrote:
> > >> > > >> >
> > >> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > >> > > >> mageshn@confluent.io
> > >> > > >> >> >
> > >> > > >> >> wrote:
> > >> > > >> >>
> > >> > > >> >> > Hi Randall,
> > >> > > >> >> >
> > >> > > >> >> > Thanks a lot for your feedback.
> > >> > > >> >> >
> > >> > > >> >> > I will update the KIP to reflect your comments in (1),
> (2),
> > >> (7)
> > >> > and
> > >> > > >> (8).
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> Looking forward to these.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > For comment # (3) , while it would be great to
> automatically
> > >> > > >> configure
> > >> > > >> >> the
> > >> > > >> >> > Rest Extensions, I would prefer that to be specified
> > >> explicitly.
> > >> > > Lets
> > >> > > >> >> > assume a connector archive includes a implementation for
> the
> > >> > > >> >> RestExtension
> > >> > > >> >> > to do authentication using some header. We don't want this
> > to
> > >> be
> > >> > > >> >> > automatically included. Having said that I think that the
> > >> config
> > >> > > key
> > >> > > >> >> name
> > >> > > >> >> > should probably be changed to something like
> > "rest.extension"
> > >> or
> > >> > > >> >> > "rest.extension.class".
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> That's a good point. I do like `rest.extension.class` (or
> > >> > > `..classes`?)
> > >> > > >> >> much more than `rest.plugins`.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > For the comment regarding the resource loading into
> jersey,
> > I
> > >> > have
> > >> > > >> the
> > >> > > >> >> > following proposal
> > >> > > >> >> >
> > >> > > >> >> > Create an implementation of Configurable(
> > >> > > >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> > Config
> > >> > > >> urable.html
> > >> > > >> >> )
> > >> > > >> >> > that delegates to ResourceConfig. In the
> > >> ConnectRestExtension, we
> > >> > > >> would
> > >> > > >> >> > expose only Configurable which is sufficient enough to
> > >> register
> > >> > new
> > >> > > >> >> > resources. In the new implementation, we will check if the
> > >> > resource
> > >> > > >> is
> > >> > > >> >> > already registered using ResourceConfig.isRegistered()
> > method
> > >> and
> > >> > > >> log a
> > >> > > >> >> > warning if the resource is already registered. This will
> > make
> > >> it
> > >> > a
> > >> > > >> >> > deterministic behavior and avoid any potential
> > >> re-registrations.
> > >> > > Let
> > >> > > >> me
> > >> > > >> >> > know your thoughts on these.
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> This sounds a good idea. Is it as flexible as the current
> > >> proposal?
> > >> > > If
> > >> > > >> >> not,
> > >> > > >> >> then I'd love to see how this affects the public APIs.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > Thanks
> > >> > > >> >> > Magesh
> > >> > > >> >> >
> > >> > > >> >> >
> > >> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> > >> > rhauch@gmail.com>
> > >> > > >> >> wrote:
> > >> > > >> >> >
> > >> > > >> >> > > Very nice proposal, Magesh. I like the approach and the
> > new
> > >> > > >> concepts
> > >> > > >> >> and
> > >> > > >> >> > > interfaces, but I do have a few comments/suggestions
> about
> > >> some
> > >> > > >> >> specific
> > >> > > >> >> > > details:
> > >> > > >> >> > >
> > >> > > >> >> > >    1. In the "Motivation" section, perhaps it makes
> sense
> > to
> > >> > > >> briefly
> > >> > > >> >> > >    describe one or two somewhat concrete examples of how
> > >> this
> > >> > is
> > >> > > >> >> useful.
> > >> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> > >> > briefly
> > >> > > >> >> describe
> > >> > > >> >> > >    each of the interfaces, what they represent, and
> which
> > >> are
> > >> > > >> >> implemented
> > >> > > >> >> > > by
> > >> > > >> >> > >    the framework vs implemented by an extension. I think
> > >> it'd
> > >> > > help
> > >> > > >> to
> > >> > > >> >> > > explain
> > >> > > >> >> > >    that only the `ConnectRestPlugin` needs to be
> > >> implemented,
> > >> > and
> > >> > > >> the
> > >> > > >> >> > rest
> > >> > > >> >> > >    will be provided by the framework. I know the next
> > >> section
> > >> > > goes
> > >> > > >> >> into
> > >> > > >> >> > it
> > >> > > >> >> > > a
> > >> > > >> >> > >    bit, but it'd be useful in this section when first
> > >> talking
> > >> > > about
> > >> > > >> >> the
> > >> > > >> >> > new
> > >> > > >> >> > >    interfaces.
> > >> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't
> > >> think we
> > >> > > >> should
> > >> > > >> >> > >    introduce a "rest.plugins" configuration property.
> > >> Instead,
> > >> > > can
> > >> > > >> we
> > >> > > >> >> not
> > >> > > >> >> > > just
> > >> > > >> >> > >    instantiate and call all of the ConnectRestPlugins
> that
> > >> we
> > >> > > find
> > >> > > >> on
> > >> > > >> >> the
> > >> > > >> >> > >    plugin path? Besides, it seems too close to the
> > >> > `plugin.path`
> > >> > > >> >> > > configuration
> > >> > > >> >> > >    property.
> > >> > > >> >> > >    4. Why would the implementation register Connect
> > >> resources
> > >> > > >> *after*
> > >> > > >> >> the
> > >> > > >> >> > >    plugins, if Jersey currently registers only the first
> > >> one?
> > >> > The
> > >> > > >> >> > "Rejected
> > >> > > >> >> > >    Alternatives" mentions why, but this section should
> be
> > >> > > explicit
> > >> > > >> >> about
> > >> > > >> >> > > why.
> > >> > > >> >> > >    For example, "The plugin's would be registered in the
> > >> > > >> >> > >    RestServer.start(Herder herder) method before
> > registering
> > >> > the
> > >> > > >> >> default
> > >> > > >> >> > >    Connect resources, which ensures that plugins cannot
> > >> remove
> > >> > > >> Connect
> > >> > > >> >> > >    resources."
> > >> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
> > >> > > re-register
> > >> > > >> the
> > >> > > >> >> > >    default Connect Resources. This could potentially
> lead
> > to
> > >> > > >> >> unexpected
> > >> > > >> >> > >    errors." First, we should not say "recommended" and
> > >> should
> > >> > > just
> > >> > > >> say
> > >> > > >> >> > > plugins
> > >> > > >> >> > >    should not register any resources that conflict with
> > the
> > >> > > >> built-in
> > >> > > >> >> > > Connect
> > >> > > >> >> > >    resources. Second, if the worker does find conflicts,
> > >> can we
> > >> > > >> just
> > >> > > >> >> > remove
> > >> > > >> >> > >    them before adding the built-in Connect resources?
> > >> > > >> >> > >    6. Is it possible for implementations to check
> whether
> > >> > > resources
> > >> > > >> >> > already
> > >> > > >> >> > >    exist before registering their own? If so, we should
> > >> > recommend
> > >> > > >> that
> > >> > > >> >> > >    implementations do this and log any problems.
> > >> > > >> >> > >    7. We should be explicit that the "Service Provider"
> is
> > >> > Java's
> > >> > > >> >> Service
> > >> > > >> >> > >    Provider API. We also need to be explicit that an
> > >> > > implementation
> > >> > > >> >> must
> > >> > > >> >> > >    provide a `META-INF/services/org.apache.
> kafka.connect.
> > >> > > >> >> > > ConnectRestPlugin`
> > >> > > >> >> > >    file (or whatever the package name of the
> > >> > `ConnectRestPlugin`
> > >> > > >> will
> > >> > > >> >> be)
> > >> > > >> >> > > with
> > >> > > >> >> > >    the fully-qualified name of the implementation
> > class(es).
> > >> > > >> >> > >    8. The example should include the META-INF file
> > required
> > >> by
> > >> > > the
> > >> > > >> >> > Service
> > >> > > >> >> > >    Provider API.
> > >> > > >> >> > >
> > >> > > >> >> > > Again, overall this is really great!
> > >> > > >> >> > >
> > >> > > >> >> > > Best regards,
> > >> > > >> >> > >
> > >> > > >> >> > > Randall
> > >> > > >> >> > >
> > >> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > >> > > >> >> > mageshn@confluent.io>
> > >> > > >> >> > > wrote:
> > >> > > >> >> > >
> > >> > > >> >> > > > Hi,
> > >> > > >> >> > > >
> > >> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin
> to
> > >> add
> > >> > > the
> > >> > > >> >> > ability
> > >> > > >> >> > > to
> > >> > > >> >> > > > provide Rest Extensions to Connect Rest API.
> > >> > > >> >> > > >
> > >> > > >> >> > > > https://cwiki.apache.org/
> confluence/display/KAFKA/KIP-
> > >> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> > >> > > >> >> > > >
> > >> > > >> >> > > > Please take a look. Your feedback is appreciated.
> > >> > > >> >> > > >
> > >> > > >> >> > > > Thanks,
> > >> > > >> >> > > >
> > >> > > >> >> > > > Magesh
> > >> > > >> >> > > >
> > >> > > >> >> > >
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >
> > >> > > >> >
> > >> > > >>
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

A few very minor suggestions:


   1. There are a few formatting issues with paragraphs that use a
   monospace font. Minor, but it would be nice to fix.
   2. Would be nice to link to the PR
   3. Do we need the org.apache.kafka.connect.rest.extension.entities
   package? Could we just move the two classes into the parent
   org.apache.kafka.connect.rest.extension package?
   4. This sentence "The above approach helps alleviate any issues that
   could arise if Extension accidentally reregister the" is cut off.
   5. The "ConnectRestExtensionContext.configure(...)" method's JavaDoc
   should describe the behaviors that are mentioned in the "Rest Extension
   Integration with Connect" section; e.g., behavior when an extension adds a
   resource that is already registered, whether unregistering works, etc.
   Also, ideally the "close()" method would have JavaDoc that explained when
   it is called (e.g., no other methods will be called on the extension after
   this, etc.).
   6. Packaging requirements are different for this component vs
   connectors, transformations, and converters, since this now mandates the
   Service Loader manifest file. This should be called out more explicitly.
   7. It'd be nice if the example included how extension-specific config
   properties are to be defined in the worker configuration file.

As I said, these are all minor suggestions that only affect the KIP
document. Once these are fixed, I think this is ready to move to voting.

Best regards,

Randall

On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Randall- I think I have addressed all the comments. Let me know if we can
> take this to Vote.
>
> Thanks
> Magesh
>
> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi All,
> >
> > I have updated the KIP to reflect changes based on the PR
> > https://github.com/apache/kafka/pull/4931. Its mostly has minor changes
> > to the interfaces and includes details on packages for the interfaces and
> > the classes. Let me know your thoughts.
> >
> > Thanks
> > Magesh
> >
> > On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> wrote:
> >
> >> Great work, Magesh. I like the overall approach a lot, so I left some
> >> pretty nuanced comments about specific details.
> >>
> >> Best regards,
> >>
> >> Randall
> >>
> >> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> mageshn@confluent.io>
> >> wrote:
> >>
> >> > Thanks Randall for your thoughts. I have created a replica of the
> >> required
> >> > entities in the draft implementation. If you can take a look at the PR
> >> and
> >> > let me know your thoughts, I will update the KIP to reflect the same
> >> >
> >> > https://github.com/apache/kafka/pull/4931
> >> >
> >> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> >> wrote:
> >> >
> >> > > Magesh, I think our last emails cross in mid-stream.
> >> > >
> >> > > We definitely want to put the new public interfaces/classes in the
> API
> >> > > module, and implementation in the runtime module. Yes, this will
> >> affect
> >> > the
> >> > > design, since for example we don't want to expose runtime types to
> the
> >> > API,
> >> > > and we want to prevent breaking changes. We don't really want to
> move
> >> the
> >> > > REST entities if we don't have to, since that may break projects
> that
> >> are
> >> > > extending the runtime module -- even though the runtime module is
> not
> >> a
> >> > > public API we still want to _try_ to change things.
> >> > >
> >> > > Do you want to try to create a prototype to see what kind of impact
> >> and
> >> > > choices we'll have to make?
> >> > >
> >> > > Best regards,
> >> > >
> >> > > Randall
> >> > >
> >> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com>
> >> > wrote:
> >> > >
> >> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
> >> > concerns,
> >> > > > though I have one more: we should specify the package names for
> all
> >> new
> >> > > > interfaces/classes.
> >> > > >
> >> > > > I'm looking forward to more feedback from others.
> >> > > >
> >> > > > Best regards,
> >> > > >
> >> > > > Randall
> >> > > >
> >> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> >> > > mageshn@confluent.io>
> >> > > > wrote:
> >> > > >
> >> > > >> Hi All,
> >> > > >>
> >> > > >> I have updated the KIP with following changes
> >> > > >>
> >> > > >>    1. Expanded the Motivation section
> >> > > >>    2. Included details about the interface in the public
> interface
> >> > > section
> >> > > >>    3. Modified the config name to rest.extension.classes
> >> > > >>    4. Modified the ConnectRestExtension to include Configurable
> >> > instead
> >> > > of
> >> > > >>    ResourceConfig
> >> > > >>    5. Modified the "Rest Extension Integration with Connect" in
> >> > > "Proposed
> >> > > >>    Approach" to include a new Custom implementation for
> >> Configurable
> >> > > >>    6. Provided examples for the Java Service provider mechanism
> >> > > >>    7. Included a reference implementation in scope
> >> > > >>
> >> > > >> Kindly let me know your thoughts on the updates.
> >> > > >>
> >> > > >> Thanks
> >> > > >> Magesh
> >> > > >>
> >> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> >> > > mageshn@confluent.io
> >> > > >> >
> >> > > >> wrote:
> >> > > >>
> >> > > >> > Hi Randall,
> >> > > >> >
> >> > > >> > Thanks for your feedback. I also would like to go with
> >> > > >> > rest.extension.classes`.
> >> > > >> >
> >> > > >> > For exposing Configurable, my original intention was just to
> >> expose
> >> > > that
> >> > > >> > to the extension because that's all one needs to register JAX
> RS
> >> > > >> resources.
> >> > > >> > The fact that we use Jersey shouldn't even be exposed in the
> >> > > interface.
> >> > > >> > Hence it doesn't affect the public API by any means.
> >> > > >> >
> >> > > >> > I will update the KIP and let everyone know.
> >> > > >> >
> >> > > >> > Thanks
> >> > > >> > Magesh
> >> > > >> >
> >> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> rhauch@gmail.com
> >> >
> >> > > >> wrote:
> >> > > >> >
> >> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> >> > > >> mageshn@confluent.io
> >> > > >> >> >
> >> > > >> >> wrote:
> >> > > >> >>
> >> > > >> >> > Hi Randall,
> >> > > >> >> >
> >> > > >> >> > Thanks a lot for your feedback.
> >> > > >> >> >
> >> > > >> >> > I will update the KIP to reflect your comments in (1), (2),
> >> (7)
> >> > and
> >> > > >> (8).
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> Looking forward to these.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > For comment # (3) , while it would be great to automatically
> >> > > >> configure
> >> > > >> >> the
> >> > > >> >> > Rest Extensions, I would prefer that to be specified
> >> explicitly.
> >> > > Lets
> >> > > >> >> > assume a connector archive includes a implementation for the
> >> > > >> >> RestExtension
> >> > > >> >> > to do authentication using some header. We don't want this
> to
> >> be
> >> > > >> >> > automatically included. Having said that I think that the
> >> config
> >> > > key
> >> > > >> >> name
> >> > > >> >> > should probably be changed to something like
> "rest.extension"
> >> or
> >> > > >> >> > "rest.extension.class".
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> That's a good point. I do like `rest.extension.class` (or
> >> > > `..classes`?)
> >> > > >> >> much more than `rest.plugins`.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > For the comment regarding the resource loading into jersey,
> I
> >> > have
> >> > > >> the
> >> > > >> >> > following proposal
> >> > > >> >> >
> >> > > >> >> > Create an implementation of Configurable(
> >> > > >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> Config
> >> > > >> urable.html
> >> > > >> >> )
> >> > > >> >> > that delegates to ResourceConfig. In the
> >> ConnectRestExtension, we
> >> > > >> would
> >> > > >> >> > expose only Configurable which is sufficient enough to
> >> register
> >> > new
> >> > > >> >> > resources. In the new implementation, we will check if the
> >> > resource
> >> > > >> is
> >> > > >> >> > already registered using ResourceConfig.isRegistered()
> method
> >> and
> >> > > >> log a
> >> > > >> >> > warning if the resource is already registered. This will
> make
> >> it
> >> > a
> >> > > >> >> > deterministic behavior and avoid any potential
> >> re-registrations.
> >> > > Let
> >> > > >> me
> >> > > >> >> > know your thoughts on these.
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> This sounds a good idea. Is it as flexible as the current
> >> proposal?
> >> > > If
> >> > > >> >> not,
> >> > > >> >> then I'd love to see how this affects the public APIs.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > Thanks
> >> > > >> >> > Magesh
> >> > > >> >> >
> >> > > >> >> >
> >> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> >> > rhauch@gmail.com>
> >> > > >> >> wrote:
> >> > > >> >> >
> >> > > >> >> > > Very nice proposal, Magesh. I like the approach and the
> new
> >> > > >> concepts
> >> > > >> >> and
> >> > > >> >> > > interfaces, but I do have a few comments/suggestions about
> >> some
> >> > > >> >> specific
> >> > > >> >> > > details:
> >> > > >> >> > >
> >> > > >> >> > >    1. In the "Motivation" section, perhaps it makes sense
> to
> >> > > >> briefly
> >> > > >> >> > >    describe one or two somewhat concrete examples of how
> >> this
> >> > is
> >> > > >> >> useful.
> >> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> >> > briefly
> >> > > >> >> describe
> >> > > >> >> > >    each of the interfaces, what they represent, and which
> >> are
> >> > > >> >> implemented
> >> > > >> >> > > by
> >> > > >> >> > >    the framework vs implemented by an extension. I think
> >> it'd
> >> > > help
> >> > > >> to
> >> > > >> >> > > explain
> >> > > >> >> > >    that only the `ConnectRestPlugin` needs to be
> >> implemented,
> >> > and
> >> > > >> the
> >> > > >> >> > rest
> >> > > >> >> > >    will be provided by the framework. I know the next
> >> section
> >> > > goes
> >> > > >> >> into
> >> > > >> >> > it
> >> > > >> >> > > a
> >> > > >> >> > >    bit, but it'd be useful in this section when first
> >> talking
> >> > > about
> >> > > >> >> the
> >> > > >> >> > new
> >> > > >> >> > >    interfaces.
> >> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't
> >> think we
> >> > > >> should
> >> > > >> >> > >    introduce a "rest.plugins" configuration property.
> >> Instead,
> >> > > can
> >> > > >> we
> >> > > >> >> not
> >> > > >> >> > > just
> >> > > >> >> > >    instantiate and call all of the ConnectRestPlugins that
> >> we
> >> > > find
> >> > > >> on
> >> > > >> >> the
> >> > > >> >> > >    plugin path? Besides, it seems too close to the
> >> > `plugin.path`
> >> > > >> >> > > configuration
> >> > > >> >> > >    property.
> >> > > >> >> > >    4. Why would the implementation register Connect
> >> resources
> >> > > >> *after*
> >> > > >> >> the
> >> > > >> >> > >    plugins, if Jersey currently registers only the first
> >> one?
> >> > The
> >> > > >> >> > "Rejected
> >> > > >> >> > >    Alternatives" mentions why, but this section should be
> >> > > explicit
> >> > > >> >> about
> >> > > >> >> > > why.
> >> > > >> >> > >    For example, "The plugin's would be registered in the
> >> > > >> >> > >    RestServer.start(Herder herder) method before
> registering
> >> > the
> >> > > >> >> default
> >> > > >> >> > >    Connect resources, which ensures that plugins cannot
> >> remove
> >> > > >> Connect
> >> > > >> >> > >    resources."
> >> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
> >> > > re-register
> >> > > >> the
> >> > > >> >> > >    default Connect Resources. This could potentially lead
> to
> >> > > >> >> unexpected
> >> > > >> >> > >    errors." First, we should not say "recommended" and
> >> should
> >> > > just
> >> > > >> say
> >> > > >> >> > > plugins
> >> > > >> >> > >    should not register any resources that conflict with
> the
> >> > > >> built-in
> >> > > >> >> > > Connect
> >> > > >> >> > >    resources. Second, if the worker does find conflicts,
> >> can we
> >> > > >> just
> >> > > >> >> > remove
> >> > > >> >> > >    them before adding the built-in Connect resources?
> >> > > >> >> > >    6. Is it possible for implementations to check whether
> >> > > resources
> >> > > >> >> > already
> >> > > >> >> > >    exist before registering their own? If so, we should
> >> > recommend
> >> > > >> that
> >> > > >> >> > >    implementations do this and log any problems.
> >> > > >> >> > >    7. We should be explicit that the "Service Provider" is
> >> > Java's
> >> > > >> >> Service
> >> > > >> >> > >    Provider API. We also need to be explicit that an
> >> > > implementation
> >> > > >> >> must
> >> > > >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> >> > > >> >> > > ConnectRestPlugin`
> >> > > >> >> > >    file (or whatever the package name of the
> >> > `ConnectRestPlugin`
> >> > > >> will
> >> > > >> >> be)
> >> > > >> >> > > with
> >> > > >> >> > >    the fully-qualified name of the implementation
> class(es).
> >> > > >> >> > >    8. The example should include the META-INF file
> required
> >> by
> >> > > the
> >> > > >> >> > Service
> >> > > >> >> > >    Provider API.
> >> > > >> >> > >
> >> > > >> >> > > Again, overall this is really great!
> >> > > >> >> > >
> >> > > >> >> > > Best regards,
> >> > > >> >> > >
> >> > > >> >> > > Randall
> >> > > >> >> > >
> >> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> >> > > >> >> > mageshn@confluent.io>
> >> > > >> >> > > wrote:
> >> > > >> >> > >
> >> > > >> >> > > > Hi,
> >> > > >> >> > > >
> >> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to
> >> add
> >> > > the
> >> > > >> >> > ability
> >> > > >> >> > > to
> >> > > >> >> > > > provide Rest Extensions to Connect Rest API.
> >> > > >> >> > > >
> >> > > >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> >> > > >> >> > > >
> >> > > >> >> > > > Please take a look. Your feedback is appreciated.
> >> > > >> >> > > >
> >> > > >> >> > > > Thanks,
> >> > > >> >> > > >
> >> > > >> >> > > > Magesh
> >> > > >> >> > > >
> >> > > >> >> > >
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >
> >> > > >> >
> >> > > >>
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Ewen,

Thanks for your comments. I have made the changes to the package names and
also moved the nested class up in the package.

Public API would include

*org.apache.kafka.connect.rest*


-ConnectClusterState
-ConnectRestExtension
-ConnectRestExtensionContext

*org.apache.kafka.connect.health*

AbstractState
ConnectClusterState
ConnectorHealth
ConnectorState
ConnectorType
TaskState

Runtime would include the following implementations

*org.apache.kafka.connect.runtime.rest*

ConnectRestExtensionContextImpl
ConnectRestConfigurable

*org.apache.kafka.connect.runtime.health*

ConnectClusterStateImpl

Let me know your thoughts.

Thanks
Magesh

On Wed, May 16, 2018 at 3:50 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:

> Hey,
>
> Sorry for the late follow up. I just had a couple of minor questions about
> details:
>
> * Some of the public API being added is under a runtime package. But that
> would be new for public API -- currently only things under the runtime
> package use that package name. I think changing the package name to just be
> under o.a.k.connect.rest or something like that would better keep this
> distinction clear and would also help shorten it a bit -- the packages are
> getting quite deeply nested with some of the new naming.
> * The cluster state classes probably shouldn't be under a rest package.
> That's where we're exposing them for public APIs currently, but it's not
> really specific to REST stuff in any way. I think we should house those
> somewhere more generic so they won't be awkward to reuse if we decided to
> (e.g. you could imagine extensions that provide this directly for metrics.
> * Currently we have the State classes nested inside ConnectorHealth class.
> I think this makes those classes more annoying to use. Is there a reason
> for them to be nested or can we just pull them out to the same level as
> ConnectorHealth?
>
> -Ewen
>
> On Tue, May 15, 2018 at 9:30 AM Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Randall- I think I have addressed all the comments. Let me know if we can
> > take this to Vote.
> >
> > Thanks
> > Magesh
> >
> > On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <mageshn@confluent.io
> >
> > wrote:
> >
> > > Hi All,
> > >
> > > I have updated the KIP to reflect changes based on the PR
> > > https://github.com/apache/kafka/pull/4931. Its mostly has minor
> changes
> > > to the interfaces and includes details on packages for the interfaces
> and
> > > the classes. Let me know your thoughts.
> > >
> > > Thanks
> > > Magesh
> > >
> > > On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> > wrote:
> > >
> > >> Great work, Magesh. I like the overall approach a lot, so I left some
> > >> pretty nuanced comments about specific details.
> > >>
> > >> Best regards,
> > >>
> > >> Randall
> > >>
> > >> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > >> wrote:
> > >>
> > >> > Thanks Randall for your thoughts. I have created a replica of the
> > >> required
> > >> > entities in the draft implementation. If you can take a look at the
> PR
> > >> and
> > >> > let me know your thoughts, I will update the KIP to reflect the same
> > >> >
> > >> > https://github.com/apache/kafka/pull/4931
> > >> >
> > >> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> > >> wrote:
> > >> >
> > >> > > Magesh, I think our last emails cross in mid-stream.
> > >> > >
> > >> > > We definitely want to put the new public interfaces/classes in the
> > API
> > >> > > module, and implementation in the runtime module. Yes, this will
> > >> affect
> > >> > the
> > >> > > design, since for example we don't want to expose runtime types to
> > the
> > >> > API,
> > >> > > and we want to prevent breaking changes. We don't really want to
> > move
> > >> the
> > >> > > REST entities if we don't have to, since that may break projects
> > that
> > >> are
> > >> > > extending the runtime module -- even though the runtime module is
> > not
> > >> a
> > >> > > public API we still want to _try_ to change things.
> > >> > >
> > >> > > Do you want to try to create a prototype to see what kind of
> impact
> > >> and
> > >> > > choices we'll have to make?
> > >> > >
> > >> > > Best regards,
> > >> > >
> > >> > > Randall
> > >> > >
> > >> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rhauch@gmail.com
> >
> > >> > wrote:
> > >> > >
> > >> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
> > >> > concerns,
> > >> > > > though I have one more: we should specify the package names for
> > all
> > >> new
> > >> > > > interfaces/classes.
> > >> > > >
> > >> > > > I'm looking forward to more feedback from others.
> > >> > > >
> > >> > > > Best regards,
> > >> > > >
> > >> > > > Randall
> > >> > > >
> > >> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > >> > > mageshn@confluent.io>
> > >> > > > wrote:
> > >> > > >
> > >> > > >> Hi All,
> > >> > > >>
> > >> > > >> I have updated the KIP with following changes
> > >> > > >>
> > >> > > >>    1. Expanded the Motivation section
> > >> > > >>    2. Included details about the interface in the public
> > interface
> > >> > > section
> > >> > > >>    3. Modified the config name to rest.extension.classes
> > >> > > >>    4. Modified the ConnectRestExtension to include Configurable
> > >> > instead
> > >> > > of
> > >> > > >>    ResourceConfig
> > >> > > >>    5. Modified the "Rest Extension Integration with Connect" in
> > >> > > "Proposed
> > >> > > >>    Approach" to include a new Custom implementation for
> > >> Configurable
> > >> > > >>    6. Provided examples for the Java Service provider mechanism
> > >> > > >>    7. Included a reference implementation in scope
> > >> > > >>
> > >> > > >> Kindly let me know your thoughts on the updates.
> > >> > > >>
> > >> > > >> Thanks
> > >> > > >> Magesh
> > >> > > >>
> > >> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > >> > > mageshn@confluent.io
> > >> > > >> >
> > >> > > >> wrote:
> > >> > > >>
> > >> > > >> > Hi Randall,
> > >> > > >> >
> > >> > > >> > Thanks for your feedback. I also would like to go with
> > >> > > >> > rest.extension.classes`.
> > >> > > >> >
> > >> > > >> > For exposing Configurable, my original intention was just to
> > >> expose
> > >> > > that
> > >> > > >> > to the extension because that's all one needs to register JAX
> > RS
> > >> > > >> resources.
> > >> > > >> > The fact that we use Jersey shouldn't even be exposed in the
> > >> > > interface.
> > >> > > >> > Hence it doesn't affect the public API by any means.
> > >> > > >> >
> > >> > > >> > I will update the KIP and let everyone know.
> > >> > > >> >
> > >> > > >> > Thanks
> > >> > > >> > Magesh
> > >> > > >> >
> > >> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> > rhauch@gmail.com
> > >> >
> > >> > > >> wrote:
> > >> > > >> >
> > >> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > >> > > >> mageshn@confluent.io
> > >> > > >> >> >
> > >> > > >> >> wrote:
> > >> > > >> >>
> > >> > > >> >> > Hi Randall,
> > >> > > >> >> >
> > >> > > >> >> > Thanks a lot for your feedback.
> > >> > > >> >> >
> > >> > > >> >> > I will update the KIP to reflect your comments in (1),
> (2),
> > >> (7)
> > >> > and
> > >> > > >> (8).
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> Looking forward to these.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > For comment # (3) , while it would be great to
> automatically
> > >> > > >> configure
> > >> > > >> >> the
> > >> > > >> >> > Rest Extensions, I would prefer that to be specified
> > >> explicitly.
> > >> > > Lets
> > >> > > >> >> > assume a connector archive includes a implementation for
> the
> > >> > > >> >> RestExtension
> > >> > > >> >> > to do authentication using some header. We don't want this
> > to
> > >> be
> > >> > > >> >> > automatically included. Having said that I think that the
> > >> config
> > >> > > key
> > >> > > >> >> name
> > >> > > >> >> > should probably be changed to something like
> > "rest.extension"
> > >> or
> > >> > > >> >> > "rest.extension.class".
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> That's a good point. I do like `rest.extension.class` (or
> > >> > > `..classes`?)
> > >> > > >> >> much more than `rest.plugins`.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > For the comment regarding the resource loading into
> jersey,
> > I
> > >> > have
> > >> > > >> the
> > >> > > >> >> > following proposal
> > >> > > >> >> >
> > >> > > >> >> > Create an implementation of Configurable(
> > >> > > >> >> >
> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
> > >> > > >> urable.html
> > >> > > >> >> )
> > >> > > >> >> > that delegates to ResourceConfig. In the
> > >> ConnectRestExtension, we
> > >> > > >> would
> > >> > > >> >> > expose only Configurable which is sufficient enough to
> > >> register
> > >> > new
> > >> > > >> >> > resources. In the new implementation, we will check if the
> > >> > resource
> > >> > > >> is
> > >> > > >> >> > already registered using ResourceConfig.isRegistered()
> > method
> > >> and
> > >> > > >> log a
> > >> > > >> >> > warning if the resource is already registered. This will
> > make
> > >> it
> > >> > a
> > >> > > >> >> > deterministic behavior and avoid any potential
> > >> re-registrations.
> > >> > > Let
> > >> > > >> me
> > >> > > >> >> > know your thoughts on these.
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >> This sounds a good idea. Is it as flexible as the current
> > >> proposal?
> > >> > > If
> > >> > > >> >> not,
> > >> > > >> >> then I'd love to see how this affects the public APIs.
> > >> > > >> >>
> > >> > > >> >>
> > >> > > >> >> >
> > >> > > >> >> > Thanks
> > >> > > >> >> > Magesh
> > >> > > >> >> >
> > >> > > >> >> >
> > >> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> > >> > rhauch@gmail.com>
> > >> > > >> >> wrote:
> > >> > > >> >> >
> > >> > > >> >> > > Very nice proposal, Magesh. I like the approach and the
> > new
> > >> > > >> concepts
> > >> > > >> >> and
> > >> > > >> >> > > interfaces, but I do have a few comments/suggestions
> about
> > >> some
> > >> > > >> >> specific
> > >> > > >> >> > > details:
> > >> > > >> >> > >
> > >> > > >> >> > >    1. In the "Motivation" section, perhaps it makes
> sense
> > to
> > >> > > >> briefly
> > >> > > >> >> > >    describe one or two somewhat concrete examples of how
> > >> this
> > >> > is
> > >> > > >> >> useful.
> > >> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> > >> > briefly
> > >> > > >> >> describe
> > >> > > >> >> > >    each of the interfaces, what they represent, and
> which
> > >> are
> > >> > > >> >> implemented
> > >> > > >> >> > > by
> > >> > > >> >> > >    the framework vs implemented by an extension. I think
> > >> it'd
> > >> > > help
> > >> > > >> to
> > >> > > >> >> > > explain
> > >> > > >> >> > >    that only the `ConnectRestPlugin` needs to be
> > >> implemented,
> > >> > and
> > >> > > >> the
> > >> > > >> >> > rest
> > >> > > >> >> > >    will be provided by the framework. I know the next
> > >> section
> > >> > > goes
> > >> > > >> >> into
> > >> > > >> >> > it
> > >> > > >> >> > > a
> > >> > > >> >> > >    bit, but it'd be useful in this section when first
> > >> talking
> > >> > > about
> > >> > > >> >> the
> > >> > > >> >> > new
> > >> > > >> >> > >    interfaces.
> > >> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't
> > >> think we
> > >> > > >> should
> > >> > > >> >> > >    introduce a "rest.plugins" configuration property.
> > >> Instead,
> > >> > > can
> > >> > > >> we
> > >> > > >> >> not
> > >> > > >> >> > > just
> > >> > > >> >> > >    instantiate and call all of the ConnectRestPlugins
> that
> > >> we
> > >> > > find
> > >> > > >> on
> > >> > > >> >> the
> > >> > > >> >> > >    plugin path? Besides, it seems too close to the
> > >> > `plugin.path`
> > >> > > >> >> > > configuration
> > >> > > >> >> > >    property.
> > >> > > >> >> > >    4. Why would the implementation register Connect
> > >> resources
> > >> > > >> *after*
> > >> > > >> >> the
> > >> > > >> >> > >    plugins, if Jersey currently registers only the first
> > >> one?
> > >> > The
> > >> > > >> >> > "Rejected
> > >> > > >> >> > >    Alternatives" mentions why, but this section should
> be
> > >> > > explicit
> > >> > > >> >> about
> > >> > > >> >> > > why.
> > >> > > >> >> > >    For example, "The plugin's would be registered in the
> > >> > > >> >> > >    RestServer.start(Herder herder) method before
> > registering
> > >> > the
> > >> > > >> >> default
> > >> > > >> >> > >    Connect resources, which ensures that plugins cannot
> > >> remove
> > >> > > >> Connect
> > >> > > >> >> > >    resources."
> > >> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
> > >> > > re-register
> > >> > > >> the
> > >> > > >> >> > >    default Connect Resources. This could potentially
> lead
> > to
> > >> > > >> >> unexpected
> > >> > > >> >> > >    errors." First, we should not say "recommended" and
> > >> should
> > >> > > just
> > >> > > >> say
> > >> > > >> >> > > plugins
> > >> > > >> >> > >    should not register any resources that conflict with
> > the
> > >> > > >> built-in
> > >> > > >> >> > > Connect
> > >> > > >> >> > >    resources. Second, if the worker does find conflicts,
> > >> can we
> > >> > > >> just
> > >> > > >> >> > remove
> > >> > > >> >> > >    them before adding the built-in Connect resources?
> > >> > > >> >> > >    6. Is it possible for implementations to check
> whether
> > >> > > resources
> > >> > > >> >> > already
> > >> > > >> >> > >    exist before registering their own? If so, we should
> > >> > recommend
> > >> > > >> that
> > >> > > >> >> > >    implementations do this and log any problems.
> > >> > > >> >> > >    7. We should be explicit that the "Service Provider"
> is
> > >> > Java's
> > >> > > >> >> Service
> > >> > > >> >> > >    Provider API. We also need to be explicit that an
> > >> > > implementation
> > >> > > >> >> must
> > >> > > >> >> > >    provide a `META-INF/services/org.apache.
> kafka.connect.
> > >> > > >> >> > > ConnectRestPlugin`
> > >> > > >> >> > >    file (or whatever the package name of the
> > >> > `ConnectRestPlugin`
> > >> > > >> will
> > >> > > >> >> be)
> > >> > > >> >> > > with
> > >> > > >> >> > >    the fully-qualified name of the implementation
> > class(es).
> > >> > > >> >> > >    8. The example should include the META-INF file
> > required
> > >> by
> > >> > > the
> > >> > > >> >> > Service
> > >> > > >> >> > >    Provider API.
> > >> > > >> >> > >
> > >> > > >> >> > > Again, overall this is really great!
> > >> > > >> >> > >
> > >> > > >> >> > > Best regards,
> > >> > > >> >> > >
> > >> > > >> >> > > Randall
> > >> > > >> >> > >
> > >> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > >> > > >> >> > mageshn@confluent.io>
> > >> > > >> >> > > wrote:
> > >> > > >> >> > >
> > >> > > >> >> > > > Hi,
> > >> > > >> >> > > >
> > >> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin
> to
> > >> add
> > >> > > the
> > >> > > >> >> > ability
> > >> > > >> >> > > to
> > >> > > >> >> > > > provide Rest Extensions to Connect Rest API.
> > >> > > >> >> > > >
> > >> > > >> >> > > > https://cwiki.apache.org/
> confluence/display/KAFKA/KIP-
> > >> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> > >> > > >> >> > > >
> > >> > > >> >> > > > Please take a look. Your feedback is appreciated.
> > >> > > >> >> > > >
> > >> > > >> >> > > > Thanks,
> > >> > > >> >> > > >
> > >> > > >> >> > > > Magesh
> > >> > > >> >> > > >
> > >> > > >> >> > >
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >
> > >> > > >> >
> > >> > > >>
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Ewen Cheslack-Postava <ew...@confluent.io>]

Hey,

Sorry for the late follow up. I just had a couple of minor questions about
details:

* Some of the public API being added is under a runtime package. But that
would be new for public API -- currently only things under the runtime
package use that package name. I think changing the package name to just be
under o.a.k.connect.rest or something like that would better keep this
distinction clear and would also help shorten it a bit -- the packages are
getting quite deeply nested with some of the new naming.
* The cluster state classes probably shouldn't be under a rest package.
That's where we're exposing them for public APIs currently, but it's not
really specific to REST stuff in any way. I think we should house those
somewhere more generic so they won't be awkward to reuse if we decided to
(e.g. you could imagine extensions that provide this directly for metrics.
* Currently we have the State classes nested inside ConnectorHealth class.
I think this makes those classes more annoying to use. Is there a reason
for them to be nested or can we just pull them out to the same level as
ConnectorHealth?

-Ewen

On Tue, May 15, 2018 at 9:30 AM Magesh Nandakumar <ma...@confluent.io>
wrote:

> Randall- I think I have addressed all the comments. Let me know if we can
> take this to Vote.
>
> Thanks
> Magesh
>
> On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi All,
> >
> > I have updated the KIP to reflect changes based on the PR
> > https://github.com/apache/kafka/pull/4931. Its mostly has minor changes
> > to the interfaces and includes details on packages for the interfaces and
> > the classes. Let me know your thoughts.
> >
> > Thanks
> > Magesh
> >
> > On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com>
> wrote:
> >
> >> Great work, Magesh. I like the overall approach a lot, so I left some
> >> pretty nuanced comments about specific details.
> >>
> >> Best regards,
> >>
> >> Randall
> >>
> >> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <
> mageshn@confluent.io>
> >> wrote:
> >>
> >> > Thanks Randall for your thoughts. I have created a replica of the
> >> required
> >> > entities in the draft implementation. If you can take a look at the PR
> >> and
> >> > let me know your thoughts, I will update the KIP to reflect the same
> >> >
> >> > https://github.com/apache/kafka/pull/4931
> >> >
> >> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> >> wrote:
> >> >
> >> > > Magesh, I think our last emails cross in mid-stream.
> >> > >
> >> > > We definitely want to put the new public interfaces/classes in the
> API
> >> > > module, and implementation in the runtime module. Yes, this will
> >> affect
> >> > the
> >> > > design, since for example we don't want to expose runtime types to
> the
> >> > API,
> >> > > and we want to prevent breaking changes. We don't really want to
> move
> >> the
> >> > > REST entities if we don't have to, since that may break projects
> that
> >> are
> >> > > extending the runtime module -- even though the runtime module is
> not
> >> a
> >> > > public API we still want to _try_ to change things.
> >> > >
> >> > > Do you want to try to create a prototype to see what kind of impact
> >> and
> >> > > choices we'll have to make?
> >> > >
> >> > > Best regards,
> >> > >
> >> > > Randall
> >> > >
> >> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com>
> >> > wrote:
> >> > >
> >> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
> >> > concerns,
> >> > > > though I have one more: we should specify the package names for
> all
> >> new
> >> > > > interfaces/classes.
> >> > > >
> >> > > > I'm looking forward to more feedback from others.
> >> > > >
> >> > > > Best regards,
> >> > > >
> >> > > > Randall
> >> > > >
> >> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> >> > > mageshn@confluent.io>
> >> > > > wrote:
> >> > > >
> >> > > >> Hi All,
> >> > > >>
> >> > > >> I have updated the KIP with following changes
> >> > > >>
> >> > > >>    1. Expanded the Motivation section
> >> > > >>    2. Included details about the interface in the public
> interface
> >> > > section
> >> > > >>    3. Modified the config name to rest.extension.classes
> >> > > >>    4. Modified the ConnectRestExtension to include Configurable
> >> > instead
> >> > > of
> >> > > >>    ResourceConfig
> >> > > >>    5. Modified the "Rest Extension Integration with Connect" in
> >> > > "Proposed
> >> > > >>    Approach" to include a new Custom implementation for
> >> Configurable
> >> > > >>    6. Provided examples for the Java Service provider mechanism
> >> > > >>    7. Included a reference implementation in scope
> >> > > >>
> >> > > >> Kindly let me know your thoughts on the updates.
> >> > > >>
> >> > > >> Thanks
> >> > > >> Magesh
> >> > > >>
> >> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> >> > > mageshn@confluent.io
> >> > > >> >
> >> > > >> wrote:
> >> > > >>
> >> > > >> > Hi Randall,
> >> > > >> >
> >> > > >> > Thanks for your feedback. I also would like to go with
> >> > > >> > rest.extension.classes`.
> >> > > >> >
> >> > > >> > For exposing Configurable, my original intention was just to
> >> expose
> >> > > that
> >> > > >> > to the extension because that's all one needs to register JAX
> RS
> >> > > >> resources.
> >> > > >> > The fact that we use Jersey shouldn't even be exposed in the
> >> > > interface.
> >> > > >> > Hence it doesn't affect the public API by any means.
> >> > > >> >
> >> > > >> > I will update the KIP and let everyone know.
> >> > > >> >
> >> > > >> > Thanks
> >> > > >> > Magesh
> >> > > >> >
> >> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <
> rhauch@gmail.com
> >> >
> >> > > >> wrote:
> >> > > >> >
> >> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> >> > > >> mageshn@confluent.io
> >> > > >> >> >
> >> > > >> >> wrote:
> >> > > >> >>
> >> > > >> >> > Hi Randall,
> >> > > >> >> >
> >> > > >> >> > Thanks a lot for your feedback.
> >> > > >> >> >
> >> > > >> >> > I will update the KIP to reflect your comments in (1), (2),
> >> (7)
> >> > and
> >> > > >> (8).
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> Looking forward to these.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > For comment # (3) , while it would be great to automatically
> >> > > >> configure
> >> > > >> >> the
> >> > > >> >> > Rest Extensions, I would prefer that to be specified
> >> explicitly.
> >> > > Lets
> >> > > >> >> > assume a connector archive includes a implementation for the
> >> > > >> >> RestExtension
> >> > > >> >> > to do authentication using some header. We don't want this
> to
> >> be
> >> > > >> >> > automatically included. Having said that I think that the
> >> config
> >> > > key
> >> > > >> >> name
> >> > > >> >> > should probably be changed to something like
> "rest.extension"
> >> or
> >> > > >> >> > "rest.extension.class".
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> That's a good point. I do like `rest.extension.class` (or
> >> > > `..classes`?)
> >> > > >> >> much more than `rest.plugins`.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > For the comment regarding the resource loading into jersey,
> I
> >> > have
> >> > > >> the
> >> > > >> >> > following proposal
> >> > > >> >> >
> >> > > >> >> > Create an implementation of Configurable(
> >> > > >> >> >
> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
> >> > > >> urable.html
> >> > > >> >> )
> >> > > >> >> > that delegates to ResourceConfig. In the
> >> ConnectRestExtension, we
> >> > > >> would
> >> > > >> >> > expose only Configurable which is sufficient enough to
> >> register
> >> > new
> >> > > >> >> > resources. In the new implementation, we will check if the
> >> > resource
> >> > > >> is
> >> > > >> >> > already registered using ResourceConfig.isRegistered()
> method
> >> and
> >> > > >> log a
> >> > > >> >> > warning if the resource is already registered. This will
> make
> >> it
> >> > a
> >> > > >> >> > deterministic behavior and avoid any potential
> >> re-registrations.
> >> > > Let
> >> > > >> me
> >> > > >> >> > know your thoughts on these.
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >> This sounds a good idea. Is it as flexible as the current
> >> proposal?
> >> > > If
> >> > > >> >> not,
> >> > > >> >> then I'd love to see how this affects the public APIs.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > Thanks
> >> > > >> >> > Magesh
> >> > > >> >> >
> >> > > >> >> >
> >> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> >> > rhauch@gmail.com>
> >> > > >> >> wrote:
> >> > > >> >> >
> >> > > >> >> > > Very nice proposal, Magesh. I like the approach and the
> new
> >> > > >> concepts
> >> > > >> >> and
> >> > > >> >> > > interfaces, but I do have a few comments/suggestions about
> >> some
> >> > > >> >> specific
> >> > > >> >> > > details:
> >> > > >> >> > >
> >> > > >> >> > >    1. In the "Motivation" section, perhaps it makes sense
> to
> >> > > >> briefly
> >> > > >> >> > >    describe one or two somewhat concrete examples of how
> >> this
> >> > is
> >> > > >> >> useful.
> >> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> >> > briefly
> >> > > >> >> describe
> >> > > >> >> > >    each of the interfaces, what they represent, and which
> >> are
> >> > > >> >> implemented
> >> > > >> >> > > by
> >> > > >> >> > >    the framework vs implemented by an extension. I think
> >> it'd
> >> > > help
> >> > > >> to
> >> > > >> >> > > explain
> >> > > >> >> > >    that only the `ConnectRestPlugin` needs to be
> >> implemented,
> >> > and
> >> > > >> the
> >> > > >> >> > rest
> >> > > >> >> > >    will be provided by the framework. I know the next
> >> section
> >> > > goes
> >> > > >> >> into
> >> > > >> >> > it
> >> > > >> >> > > a
> >> > > >> >> > >    bit, but it'd be useful in this section when first
> >> talking
> >> > > about
> >> > > >> >> the
> >> > > >> >> > new
> >> > > >> >> > >    interfaces.
> >> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't
> >> think we
> >> > > >> should
> >> > > >> >> > >    introduce a "rest.plugins" configuration property.
> >> Instead,
> >> > > can
> >> > > >> we
> >> > > >> >> not
> >> > > >> >> > > just
> >> > > >> >> > >    instantiate and call all of the ConnectRestPlugins that
> >> we
> >> > > find
> >> > > >> on
> >> > > >> >> the
> >> > > >> >> > >    plugin path? Besides, it seems too close to the
> >> > `plugin.path`
> >> > > >> >> > > configuration
> >> > > >> >> > >    property.
> >> > > >> >> > >    4. Why would the implementation register Connect
> >> resources
> >> > > >> *after*
> >> > > >> >> the
> >> > > >> >> > >    plugins, if Jersey currently registers only the first
> >> one?
> >> > The
> >> > > >> >> > "Rejected
> >> > > >> >> > >    Alternatives" mentions why, but this section should be
> >> > > explicit
> >> > > >> >> about
> >> > > >> >> > > why.
> >> > > >> >> > >    For example, "The plugin's would be registered in the
> >> > > >> >> > >    RestServer.start(Herder herder) method before
> registering
> >> > the
> >> > > >> >> default
> >> > > >> >> > >    Connect resources, which ensures that plugins cannot
> >> remove
> >> > > >> Connect
> >> > > >> >> > >    resources."
> >> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
> >> > > re-register
> >> > > >> the
> >> > > >> >> > >    default Connect Resources. This could potentially lead
> to
> >> > > >> >> unexpected
> >> > > >> >> > >    errors." First, we should not say "recommended" and
> >> should
> >> > > just
> >> > > >> say
> >> > > >> >> > > plugins
> >> > > >> >> > >    should not register any resources that conflict with
> the
> >> > > >> built-in
> >> > > >> >> > > Connect
> >> > > >> >> > >    resources. Second, if the worker does find conflicts,
> >> can we
> >> > > >> just
> >> > > >> >> > remove
> >> > > >> >> > >    them before adding the built-in Connect resources?
> >> > > >> >> > >    6. Is it possible for implementations to check whether
> >> > > resources
> >> > > >> >> > already
> >> > > >> >> > >    exist before registering their own? If so, we should
> >> > recommend
> >> > > >> that
> >> > > >> >> > >    implementations do this and log any problems.
> >> > > >> >> > >    7. We should be explicit that the "Service Provider" is
> >> > Java's
> >> > > >> >> Service
> >> > > >> >> > >    Provider API. We also need to be explicit that an
> >> > > implementation
> >> > > >> >> must
> >> > > >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> >> > > >> >> > > ConnectRestPlugin`
> >> > > >> >> > >    file (or whatever the package name of the
> >> > `ConnectRestPlugin`
> >> > > >> will
> >> > > >> >> be)
> >> > > >> >> > > with
> >> > > >> >> > >    the fully-qualified name of the implementation
> class(es).
> >> > > >> >> > >    8. The example should include the META-INF file
> required
> >> by
> >> > > the
> >> > > >> >> > Service
> >> > > >> >> > >    Provider API.
> >> > > >> >> > >
> >> > > >> >> > > Again, overall this is really great!
> >> > > >> >> > >
> >> > > >> >> > > Best regards,
> >> > > >> >> > >
> >> > > >> >> > > Randall
> >> > > >> >> > >
> >> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> >> > > >> >> > mageshn@confluent.io>
> >> > > >> >> > > wrote:
> >> > > >> >> > >
> >> > > >> >> > > > Hi,
> >> > > >> >> > > >
> >> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to
> >> add
> >> > > the
> >> > > >> >> > ability
> >> > > >> >> > > to
> >> > > >> >> > > > provide Rest Extensions to Connect Rest API.
> >> > > >> >> > > >
> >> > > >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> >> > > >> >> > > >
> >> > > >> >> > > > Please take a look. Your feedback is appreciated.
> >> > > >> >> > > >
> >> > > >> >> > > > Thanks,
> >> > > >> >> > > >
> >> > > >> >> > > > Magesh
> >> > > >> >> > > >
> >> > > >> >> > >
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >
> >> > > >> >
> >> > > >>
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Randall- I think I have addressed all the comments. Let me know if we can
take this to Vote.

Thanks
Magesh

On Tue, May 8, 2018 at 10:12 PM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi All,
>
> I have updated the KIP to reflect changes based on the PR
> https://github.com/apache/kafka/pull/4931. Its mostly has minor changes
> to the interfaces and includes details on packages for the interfaces and
> the classes. Let me know your thoughts.
>
> Thanks
> Magesh
>
> On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com> wrote:
>
>> Great work, Magesh. I like the overall approach a lot, so I left some
>> pretty nuanced comments about specific details.
>>
>> Best regards,
>>
>> Randall
>>
>> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <ma...@confluent.io>
>> wrote:
>>
>> > Thanks Randall for your thoughts. I have created a replica of the
>> required
>> > entities in the draft implementation. If you can take a look at the PR
>> and
>> > let me know your thoughts, I will update the KIP to reflect the same
>> >
>> > https://github.com/apache/kafka/pull/4931
>> >
>> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
>> wrote:
>> >
>> > > Magesh, I think our last emails cross in mid-stream.
>> > >
>> > > We definitely want to put the new public interfaces/classes in the API
>> > > module, and implementation in the runtime module. Yes, this will
>> affect
>> > the
>> > > design, since for example we don't want to expose runtime types to the
>> > API,
>> > > and we want to prevent breaking changes. We don't really want to move
>> the
>> > > REST entities if we don't have to, since that may break projects that
>> are
>> > > extending the runtime module -- even though the runtime module is not
>> a
>> > > public API we still want to _try_ to change things.
>> > >
>> > > Do you want to try to create a prototype to see what kind of impact
>> and
>> > > choices we'll have to make?
>> > >
>> > > Best regards,
>> > >
>> > > Randall
>> > >
>> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com>
>> > wrote:
>> > >
>> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
>> > concerns,
>> > > > though I have one more: we should specify the package names for all
>> new
>> > > > interfaces/classes.
>> > > >
>> > > > I'm looking forward to more feedback from others.
>> > > >
>> > > > Best regards,
>> > > >
>> > > > Randall
>> > > >
>> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
>> > > mageshn@confluent.io>
>> > > > wrote:
>> > > >
>> > > >> Hi All,
>> > > >>
>> > > >> I have updated the KIP with following changes
>> > > >>
>> > > >>    1. Expanded the Motivation section
>> > > >>    2. Included details about the interface in the public interface
>> > > section
>> > > >>    3. Modified the config name to rest.extension.classes
>> > > >>    4. Modified the ConnectRestExtension to include Configurable
>> > instead
>> > > of
>> > > >>    ResourceConfig
>> > > >>    5. Modified the "Rest Extension Integration with Connect" in
>> > > "Proposed
>> > > >>    Approach" to include a new Custom implementation for
>> Configurable
>> > > >>    6. Provided examples for the Java Service provider mechanism
>> > > >>    7. Included a reference implementation in scope
>> > > >>
>> > > >> Kindly let me know your thoughts on the updates.
>> > > >>
>> > > >> Thanks
>> > > >> Magesh
>> > > >>
>> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
>> > > mageshn@confluent.io
>> > > >> >
>> > > >> wrote:
>> > > >>
>> > > >> > Hi Randall,
>> > > >> >
>> > > >> > Thanks for your feedback. I also would like to go with
>> > > >> > rest.extension.classes`.
>> > > >> >
>> > > >> > For exposing Configurable, my original intention was just to
>> expose
>> > > that
>> > > >> > to the extension because that's all one needs to register JAX RS
>> > > >> resources.
>> > > >> > The fact that we use Jersey shouldn't even be exposed in the
>> > > interface.
>> > > >> > Hence it doesn't affect the public API by any means.
>> > > >> >
>> > > >> > I will update the KIP and let everyone know.
>> > > >> >
>> > > >> > Thanks
>> > > >> > Magesh
>> > > >> >
>> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rhauch@gmail.com
>> >
>> > > >> wrote:
>> > > >> >
>> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
>> > > >> mageshn@confluent.io
>> > > >> >> >
>> > > >> >> wrote:
>> > > >> >>
>> > > >> >> > Hi Randall,
>> > > >> >> >
>> > > >> >> > Thanks a lot for your feedback.
>> > > >> >> >
>> > > >> >> > I will update the KIP to reflect your comments in (1), (2),
>> (7)
>> > and
>> > > >> (8).
>> > > >> >> >
>> > > >> >>
>> > > >> >> Looking forward to these.
>> > > >> >>
>> > > >> >>
>> > > >> >> >
>> > > >> >> > For comment # (3) , while it would be great to automatically
>> > > >> configure
>> > > >> >> the
>> > > >> >> > Rest Extensions, I would prefer that to be specified
>> explicitly.
>> > > Lets
>> > > >> >> > assume a connector archive includes a implementation for the
>> > > >> >> RestExtension
>> > > >> >> > to do authentication using some header. We don't want this to
>> be
>> > > >> >> > automatically included. Having said that I think that the
>> config
>> > > key
>> > > >> >> name
>> > > >> >> > should probably be changed to something like "rest.extension"
>> or
>> > > >> >> > "rest.extension.class".
>> > > >> >> >
>> > > >> >>
>> > > >> >> That's a good point. I do like `rest.extension.class` (or
>> > > `..classes`?)
>> > > >> >> much more than `rest.plugins`.
>> > > >> >>
>> > > >> >>
>> > > >> >> >
>> > > >> >> > For the comment regarding the resource loading into jersey, I
>> > have
>> > > >> the
>> > > >> >> > following proposal
>> > > >> >> >
>> > > >> >> > Create an implementation of Configurable(
>> > > >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
>> > > >> urable.html
>> > > >> >> )
>> > > >> >> > that delegates to ResourceConfig. In the
>> ConnectRestExtension, we
>> > > >> would
>> > > >> >> > expose only Configurable which is sufficient enough to
>> register
>> > new
>> > > >> >> > resources. In the new implementation, we will check if the
>> > resource
>> > > >> is
>> > > >> >> > already registered using ResourceConfig.isRegistered() method
>> and
>> > > >> log a
>> > > >> >> > warning if the resource is already registered. This will make
>> it
>> > a
>> > > >> >> > deterministic behavior and avoid any potential
>> re-registrations.
>> > > Let
>> > > >> me
>> > > >> >> > know your thoughts on these.
>> > > >> >> >
>> > > >> >>
>> > > >> >> This sounds a good idea. Is it as flexible as the current
>> proposal?
>> > > If
>> > > >> >> not,
>> > > >> >> then I'd love to see how this affects the public APIs.
>> > > >> >>
>> > > >> >>
>> > > >> >> >
>> > > >> >> > Thanks
>> > > >> >> > Magesh
>> > > >> >> >
>> > > >> >> >
>> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
>> > rhauch@gmail.com>
>> > > >> >> wrote:
>> > > >> >> >
>> > > >> >> > > Very nice proposal, Magesh. I like the approach and the new
>> > > >> concepts
>> > > >> >> and
>> > > >> >> > > interfaces, but I do have a few comments/suggestions about
>> some
>> > > >> >> specific
>> > > >> >> > > details:
>> > > >> >> > >
>> > > >> >> > >    1. In the "Motivation" section, perhaps it makes sense to
>> > > >> briefly
>> > > >> >> > >    describe one or two somewhat concrete examples of how
>> this
>> > is
>> > > >> >> useful.
>> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
>> > briefly
>> > > >> >> describe
>> > > >> >> > >    each of the interfaces, what they represent, and which
>> are
>> > > >> >> implemented
>> > > >> >> > > by
>> > > >> >> > >    the framework vs implemented by an extension. I think
>> it'd
>> > > help
>> > > >> to
>> > > >> >> > > explain
>> > > >> >> > >    that only the `ConnectRestPlugin` needs to be
>> implemented,
>> > and
>> > > >> the
>> > > >> >> > rest
>> > > >> >> > >    will be provided by the framework. I know the next
>> section
>> > > goes
>> > > >> >> into
>> > > >> >> > it
>> > > >> >> > > a
>> > > >> >> > >    bit, but it'd be useful in this section when first
>> talking
>> > > about
>> > > >> >> the
>> > > >> >> > new
>> > > >> >> > >    interfaces.
>> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't
>> think we
>> > > >> should
>> > > >> >> > >    introduce a "rest.plugins" configuration property.
>> Instead,
>> > > can
>> > > >> we
>> > > >> >> not
>> > > >> >> > > just
>> > > >> >> > >    instantiate and call all of the ConnectRestPlugins that
>> we
>> > > find
>> > > >> on
>> > > >> >> the
>> > > >> >> > >    plugin path? Besides, it seems too close to the
>> > `plugin.path`
>> > > >> >> > > configuration
>> > > >> >> > >    property.
>> > > >> >> > >    4. Why would the implementation register Connect
>> resources
>> > > >> *after*
>> > > >> >> the
>> > > >> >> > >    plugins, if Jersey currently registers only the first
>> one?
>> > The
>> > > >> >> > "Rejected
>> > > >> >> > >    Alternatives" mentions why, but this section should be
>> > > explicit
>> > > >> >> about
>> > > >> >> > > why.
>> > > >> >> > >    For example, "The plugin's would be registered in the
>> > > >> >> > >    RestServer.start(Herder herder) method before registering
>> > the
>> > > >> >> default
>> > > >> >> > >    Connect resources, which ensures that plugins cannot
>> remove
>> > > >> Connect
>> > > >> >> > >    resources."
>> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
>> > > re-register
>> > > >> the
>> > > >> >> > >    default Connect Resources. This could potentially lead to
>> > > >> >> unexpected
>> > > >> >> > >    errors." First, we should not say "recommended" and
>> should
>> > > just
>> > > >> say
>> > > >> >> > > plugins
>> > > >> >> > >    should not register any resources that conflict with the
>> > > >> built-in
>> > > >> >> > > Connect
>> > > >> >> > >    resources. Second, if the worker does find conflicts,
>> can we
>> > > >> just
>> > > >> >> > remove
>> > > >> >> > >    them before adding the built-in Connect resources?
>> > > >> >> > >    6. Is it possible for implementations to check whether
>> > > resources
>> > > >> >> > already
>> > > >> >> > >    exist before registering their own? If so, we should
>> > recommend
>> > > >> that
>> > > >> >> > >    implementations do this and log any problems.
>> > > >> >> > >    7. We should be explicit that the "Service Provider" is
>> > Java's
>> > > >> >> Service
>> > > >> >> > >    Provider API. We also need to be explicit that an
>> > > implementation
>> > > >> >> must
>> > > >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
>> > > >> >> > > ConnectRestPlugin`
>> > > >> >> > >    file (or whatever the package name of the
>> > `ConnectRestPlugin`
>> > > >> will
>> > > >> >> be)
>> > > >> >> > > with
>> > > >> >> > >    the fully-qualified name of the implementation class(es).
>> > > >> >> > >    8. The example should include the META-INF file required
>> by
>> > > the
>> > > >> >> > Service
>> > > >> >> > >    Provider API.
>> > > >> >> > >
>> > > >> >> > > Again, overall this is really great!
>> > > >> >> > >
>> > > >> >> > > Best regards,
>> > > >> >> > >
>> > > >> >> > > Randall
>> > > >> >> > >
>> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
>> > > >> >> > mageshn@confluent.io>
>> > > >> >> > > wrote:
>> > > >> >> > >
>> > > >> >> > > > Hi,
>> > > >> >> > > >
>> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to
>> add
>> > > the
>> > > >> >> > ability
>> > > >> >> > > to
>> > > >> >> > > > provide Rest Extensions to Connect Rest API.
>> > > >> >> > > >
>> > > >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
>> > > >> >> > > >
>> > > >> >> > > > Please take a look. Your feedback is appreciated.
>> > > >> >> > > >
>> > > >> >> > > > Thanks,
>> > > >> >> > > >
>> > > >> >> > > > Magesh
>> > > >> >> > > >
>> > > >> >> > >
>> > > >> >> >
>> > > >> >>
>> > > >> >
>> > > >> >
>> > > >>
>> > > >
>> > > >
>> > >
>> >
>>
>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi All,

I have updated the KIP to reflect changes based on the PR
https://github.com/apache/kafka/pull/4931. Its mostly has minor changes to
the interfaces and includes details on packages for the interfaces and the
classes. Let me know your thoughts.

Thanks
Magesh

On Fri, Apr 27, 2018 at 12:03 PM, Randall Hauch <rh...@gmail.com> wrote:

> Great work, Magesh. I like the overall approach a lot, so I left some
> pretty nuanced comments about specific details.
>
> Best regards,
>
> Randall
>
> On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Thanks Randall for your thoughts. I have created a replica of the
> required
> > entities in the draft implementation. If you can take a look at the PR
> and
> > let me know your thoughts, I will update the KIP to reflect the same
> >
> > https://github.com/apache/kafka/pull/4931
> >
> > On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com>
> wrote:
> >
> > > Magesh, I think our last emails cross in mid-stream.
> > >
> > > We definitely want to put the new public interfaces/classes in the API
> > > module, and implementation in the runtime module. Yes, this will affect
> > the
> > > design, since for example we don't want to expose runtime types to the
> > API,
> > > and we want to prevent breaking changes. We don't really want to move
> the
> > > REST entities if we don't have to, since that may break projects that
> are
> > > extending the runtime module -- even though the runtime module is not a
> > > public API we still want to _try_ to change things.
> > >
> > > Do you want to try to create a prototype to see what kind of impact and
> > > choices we'll have to make?
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com>
> > wrote:
> > >
> > > > Thanks for updating the KIP, Magesh. You've resolved all of my
> > concerns,
> > > > though I have one more: we should specify the package names for all
> new
> > > > interfaces/classes.
> > > >
> > > > I'm looking forward to more feedback from others.
> > > >
> > > > Best regards,
> > > >
> > > > Randall
> > > >
> > > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > > mageshn@confluent.io>
> > > > wrote:
> > > >
> > > >> Hi All,
> > > >>
> > > >> I have updated the KIP with following changes
> > > >>
> > > >>    1. Expanded the Motivation section
> > > >>    2. Included details about the interface in the public interface
> > > section
> > > >>    3. Modified the config name to rest.extension.classes
> > > >>    4. Modified the ConnectRestExtension to include Configurable
> > instead
> > > of
> > > >>    ResourceConfig
> > > >>    5. Modified the "Rest Extension Integration with Connect" in
> > > "Proposed
> > > >>    Approach" to include a new Custom implementation for Configurable
> > > >>    6. Provided examples for the Java Service provider mechanism
> > > >>    7. Included a reference implementation in scope
> > > >>
> > > >> Kindly let me know your thoughts on the updates.
> > > >>
> > > >> Thanks
> > > >> Magesh
> > > >>
> > > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > > mageshn@confluent.io
> > > >> >
> > > >> wrote:
> > > >>
> > > >> > Hi Randall,
> > > >> >
> > > >> > Thanks for your feedback. I also would like to go with
> > > >> > rest.extension.classes`.
> > > >> >
> > > >> > For exposing Configurable, my original intention was just to
> expose
> > > that
> > > >> > to the extension because that's all one needs to register JAX RS
> > > >> resources.
> > > >> > The fact that we use Jersey shouldn't even be exposed in the
> > > interface.
> > > >> > Hence it doesn't affect the public API by any means.
> > > >> >
> > > >> > I will update the KIP and let everyone know.
> > > >> >
> > > >> > Thanks
> > > >> > Magesh
> > > >> >
> > > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com>
> > > >> wrote:
> > > >> >
> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > > >> mageshn@confluent.io
> > > >> >> >
> > > >> >> wrote:
> > > >> >>
> > > >> >> > Hi Randall,
> > > >> >> >
> > > >> >> > Thanks a lot for your feedback.
> > > >> >> >
> > > >> >> > I will update the KIP to reflect your comments in (1), (2), (7)
> > and
> > > >> (8).
> > > >> >> >
> > > >> >>
> > > >> >> Looking forward to these.
> > > >> >>
> > > >> >>
> > > >> >> >
> > > >> >> > For comment # (3) , while it would be great to automatically
> > > >> configure
> > > >> >> the
> > > >> >> > Rest Extensions, I would prefer that to be specified
> explicitly.
> > > Lets
> > > >> >> > assume a connector archive includes a implementation for the
> > > >> >> RestExtension
> > > >> >> > to do authentication using some header. We don't want this to
> be
> > > >> >> > automatically included. Having said that I think that the
> config
> > > key
> > > >> >> name
> > > >> >> > should probably be changed to something like "rest.extension"
> or
> > > >> >> > "rest.extension.class".
> > > >> >> >
> > > >> >>
> > > >> >> That's a good point. I do like `rest.extension.class` (or
> > > `..classes`?)
> > > >> >> much more than `rest.plugins`.
> > > >> >>
> > > >> >>
> > > >> >> >
> > > >> >> > For the comment regarding the resource loading into jersey, I
> > have
> > > >> the
> > > >> >> > following proposal
> > > >> >> >
> > > >> >> > Create an implementation of Configurable(
> > > >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
> > > >> urable.html
> > > >> >> )
> > > >> >> > that delegates to ResourceConfig. In the ConnectRestExtension,
> we
> > > >> would
> > > >> >> > expose only Configurable which is sufficient enough to register
> > new
> > > >> >> > resources. In the new implementation, we will check if the
> > resource
> > > >> is
> > > >> >> > already registered using ResourceConfig.isRegistered() method
> and
> > > >> log a
> > > >> >> > warning if the resource is already registered. This will make
> it
> > a
> > > >> >> > deterministic behavior and avoid any potential
> re-registrations.
> > > Let
> > > >> me
> > > >> >> > know your thoughts on these.
> > > >> >> >
> > > >> >>
> > > >> >> This sounds a good idea. Is it as flexible as the current
> proposal?
> > > If
> > > >> >> not,
> > > >> >> then I'd love to see how this affects the public APIs.
> > > >> >>
> > > >> >>
> > > >> >> >
> > > >> >> > Thanks
> > > >> >> > Magesh
> > > >> >> >
> > > >> >> >
> > > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> > rhauch@gmail.com>
> > > >> >> wrote:
> > > >> >> >
> > > >> >> > > Very nice proposal, Magesh. I like the approach and the new
> > > >> concepts
> > > >> >> and
> > > >> >> > > interfaces, but I do have a few comments/suggestions about
> some
> > > >> >> specific
> > > >> >> > > details:
> > > >> >> > >
> > > >> >> > >    1. In the "Motivation" section, perhaps it makes sense to
> > > >> briefly
> > > >> >> > >    describe one or two somewhat concrete examples of how this
> > is
> > > >> >> useful.
> > > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> > briefly
> > > >> >> describe
> > > >> >> > >    each of the interfaces, what they represent, and which are
> > > >> >> implemented
> > > >> >> > > by
> > > >> >> > >    the framework vs implemented by an extension. I think it'd
> > > help
> > > >> to
> > > >> >> > > explain
> > > >> >> > >    that only the `ConnectRestPlugin` needs to be implemented,
> > and
> > > >> the
> > > >> >> > rest
> > > >> >> > >    will be provided by the framework. I know the next section
> > > goes
> > > >> >> into
> > > >> >> > it
> > > >> >> > > a
> > > >> >> > >    bit, but it'd be useful in this section when first talking
> > > about
> > > >> >> the
> > > >> >> > new
> > > >> >> > >    interfaces.
> > > >> >> > >    3. Also in the "Public Interfaces" section: I don't think
> we
> > > >> should
> > > >> >> > >    introduce a "rest.plugins" configuration property.
> Instead,
> > > can
> > > >> we
> > > >> >> not
> > > >> >> > > just
> > > >> >> > >    instantiate and call all of the ConnectRestPlugins that we
> > > find
> > > >> on
> > > >> >> the
> > > >> >> > >    plugin path? Besides, it seems too close to the
> > `plugin.path`
> > > >> >> > > configuration
> > > >> >> > >    property.
> > > >> >> > >    4. Why would the implementation register Connect resources
> > > >> *after*
> > > >> >> the
> > > >> >> > >    plugins, if Jersey currently registers only the first one?
> > The
> > > >> >> > "Rejected
> > > >> >> > >    Alternatives" mentions why, but this section should be
> > > explicit
> > > >> >> about
> > > >> >> > > why.
> > > >> >> > >    For example, "The plugin's would be registered in the
> > > >> >> > >    RestServer.start(Herder herder) method before registering
> > the
> > > >> >> default
> > > >> >> > >    Connect resources, which ensures that plugins cannot
> remove
> > > >> Connect
> > > >> >> > >    resources."
> > > >> >> > >    5. "Hence, it is recommended that the plugins don't
> > > re-register
> > > >> the
> > > >> >> > >    default Connect Resources. This could potentially lead to
> > > >> >> unexpected
> > > >> >> > >    errors." First, we should not say "recommended" and should
> > > just
> > > >> say
> > > >> >> > > plugins
> > > >> >> > >    should not register any resources that conflict with the
> > > >> built-in
> > > >> >> > > Connect
> > > >> >> > >    resources. Second, if the worker does find conflicts, can
> we
> > > >> just
> > > >> >> > remove
> > > >> >> > >    them before adding the built-in Connect resources?
> > > >> >> > >    6. Is it possible for implementations to check whether
> > > resources
> > > >> >> > already
> > > >> >> > >    exist before registering their own? If so, we should
> > recommend
> > > >> that
> > > >> >> > >    implementations do this and log any problems.
> > > >> >> > >    7. We should be explicit that the "Service Provider" is
> > Java's
> > > >> >> Service
> > > >> >> > >    Provider API. We also need to be explicit that an
> > > implementation
> > > >> >> must
> > > >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> > > >> >> > > ConnectRestPlugin`
> > > >> >> > >    file (or whatever the package name of the
> > `ConnectRestPlugin`
> > > >> will
> > > >> >> be)
> > > >> >> > > with
> > > >> >> > >    the fully-qualified name of the implementation class(es).
> > > >> >> > >    8. The example should include the META-INF file required
> by
> > > the
> > > >> >> > Service
> > > >> >> > >    Provider API.
> > > >> >> > >
> > > >> >> > > Again, overall this is really great!
> > > >> >> > >
> > > >> >> > > Best regards,
> > > >> >> > >
> > > >> >> > > Randall
> > > >> >> > >
> > > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > > >> >> > mageshn@confluent.io>
> > > >> >> > > wrote:
> > > >> >> > >
> > > >> >> > > > Hi,
> > > >> >> > > >
> > > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to
> add
> > > the
> > > >> >> > ability
> > > >> >> > > to
> > > >> >> > > > provide Rest Extensions to Connect Rest API.
> > > >> >> > > >
> > > >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> > > >> >> > > >
> > > >> >> > > > Please take a look. Your feedback is appreciated.
> > > >> >> > > >
> > > >> >> > > > Thanks,
> > > >> >> > > >
> > > >> >> > > > Magesh
> > > >> >> > > >
> > > >> >> > >
> > > >> >> >
> > > >> >>
> > > >> >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

Great work, Magesh. I like the overall approach a lot, so I left some
pretty nuanced comments about specific details.

Best regards,

Randall

On Wed, Apr 25, 2018 at 3:03 PM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Thanks Randall for your thoughts. I have created a replica of the required
> entities in the draft implementation. If you can take a look at the PR and
> let me know your thoughts, I will update the KIP to reflect the same
>
> https://github.com/apache/kafka/pull/4931
>
> On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com> wrote:
>
> > Magesh, I think our last emails cross in mid-stream.
> >
> > We definitely want to put the new public interfaces/classes in the API
> > module, and implementation in the runtime module. Yes, this will affect
> the
> > design, since for example we don't want to expose runtime types to the
> API,
> > and we want to prevent breaking changes. We don't really want to move the
> > REST entities if we don't have to, since that may break projects that are
> > extending the runtime module -- even though the runtime module is not a
> > public API we still want to _try_ to change things.
> >
> > Do you want to try to create a prototype to see what kind of impact and
> > choices we'll have to make?
> >
> > Best regards,
> >
> > Randall
> >
> > On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com>
> wrote:
> >
> > > Thanks for updating the KIP, Magesh. You've resolved all of my
> concerns,
> > > though I have one more: we should specify the package names for all new
> > > interfaces/classes.
> > >
> > > I'm looking forward to more feedback from others.
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > > wrote:
> > >
> > >> Hi All,
> > >>
> > >> I have updated the KIP with following changes
> > >>
> > >>    1. Expanded the Motivation section
> > >>    2. Included details about the interface in the public interface
> > section
> > >>    3. Modified the config name to rest.extension.classes
> > >>    4. Modified the ConnectRestExtension to include Configurable
> instead
> > of
> > >>    ResourceConfig
> > >>    5. Modified the "Rest Extension Integration with Connect" in
> > "Proposed
> > >>    Approach" to include a new Custom implementation for Configurable
> > >>    6. Provided examples for the Java Service provider mechanism
> > >>    7. Included a reference implementation in scope
> > >>
> > >> Kindly let me know your thoughts on the updates.
> > >>
> > >> Thanks
> > >> Magesh
> > >>
> > >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> > mageshn@confluent.io
> > >> >
> > >> wrote:
> > >>
> > >> > Hi Randall,
> > >> >
> > >> > Thanks for your feedback. I also would like to go with
> > >> > rest.extension.classes`.
> > >> >
> > >> > For exposing Configurable, my original intention was just to expose
> > that
> > >> > to the extension because that's all one needs to register JAX RS
> > >> resources.
> > >> > The fact that we use Jersey shouldn't even be exposed in the
> > interface.
> > >> > Hence it doesn't affect the public API by any means.
> > >> >
> > >> > I will update the KIP and let everyone know.
> > >> >
> > >> > Thanks
> > >> > Magesh
> > >> >
> > >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com>
> > >> wrote:
> > >> >
> > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> > >> mageshn@confluent.io
> > >> >> >
> > >> >> wrote:
> > >> >>
> > >> >> > Hi Randall,
> > >> >> >
> > >> >> > Thanks a lot for your feedback.
> > >> >> >
> > >> >> > I will update the KIP to reflect your comments in (1), (2), (7)
> and
> > >> (8).
> > >> >> >
> > >> >>
> > >> >> Looking forward to these.
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > For comment # (3) , while it would be great to automatically
> > >> configure
> > >> >> the
> > >> >> > Rest Extensions, I would prefer that to be specified explicitly.
> > Lets
> > >> >> > assume a connector archive includes a implementation for the
> > >> >> RestExtension
> > >> >> > to do authentication using some header. We don't want this to be
> > >> >> > automatically included. Having said that I think that the config
> > key
> > >> >> name
> > >> >> > should probably be changed to something like "rest.extension" or
> > >> >> > "rest.extension.class".
> > >> >> >
> > >> >>
> > >> >> That's a good point. I do like `rest.extension.class` (or
> > `..classes`?)
> > >> >> much more than `rest.plugins`.
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > For the comment regarding the resource loading into jersey, I
> have
> > >> the
> > >> >> > following proposal
> > >> >> >
> > >> >> > Create an implementation of Configurable(
> > >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
> > >> urable.html
> > >> >> )
> > >> >> > that delegates to ResourceConfig. In the ConnectRestExtension, we
> > >> would
> > >> >> > expose only Configurable which is sufficient enough to register
> new
> > >> >> > resources. In the new implementation, we will check if the
> resource
> > >> is
> > >> >> > already registered using ResourceConfig.isRegistered() method and
> > >> log a
> > >> >> > warning if the resource is already registered. This will make it
> a
> > >> >> > deterministic behavior and avoid any potential re-registrations.
> > Let
> > >> me
> > >> >> > know your thoughts on these.
> > >> >> >
> > >> >>
> > >> >> This sounds a good idea. Is it as flexible as the current proposal?
> > If
> > >> >> not,
> > >> >> then I'd love to see how this affects the public APIs.
> > >> >>
> > >> >>
> > >> >> >
> > >> >> > Thanks
> > >> >> > Magesh
> > >> >> >
> > >> >> >
> > >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <
> rhauch@gmail.com>
> > >> >> wrote:
> > >> >> >
> > >> >> > > Very nice proposal, Magesh. I like the approach and the new
> > >> concepts
> > >> >> and
> > >> >> > > interfaces, but I do have a few comments/suggestions about some
> > >> >> specific
> > >> >> > > details:
> > >> >> > >
> > >> >> > >    1. In the "Motivation" section, perhaps it makes sense to
> > >> briefly
> > >> >> > >    describe one or two somewhat concrete examples of how this
> is
> > >> >> useful.
> > >> >> > >    2. Maybe in the "Public Interfaces" section you could
> briefly
> > >> >> describe
> > >> >> > >    each of the interfaces, what they represent, and which are
> > >> >> implemented
> > >> >> > > by
> > >> >> > >    the framework vs implemented by an extension. I think it'd
> > help
> > >> to
> > >> >> > > explain
> > >> >> > >    that only the `ConnectRestPlugin` needs to be implemented,
> and
> > >> the
> > >> >> > rest
> > >> >> > >    will be provided by the framework. I know the next section
> > goes
> > >> >> into
> > >> >> > it
> > >> >> > > a
> > >> >> > >    bit, but it'd be useful in this section when first talking
> > about
> > >> >> the
> > >> >> > new
> > >> >> > >    interfaces.
> > >> >> > >    3. Also in the "Public Interfaces" section: I don't think we
> > >> should
> > >> >> > >    introduce a "rest.plugins" configuration property. Instead,
> > can
> > >> we
> > >> >> not
> > >> >> > > just
> > >> >> > >    instantiate and call all of the ConnectRestPlugins that we
> > find
> > >> on
> > >> >> the
> > >> >> > >    plugin path? Besides, it seems too close to the
> `plugin.path`
> > >> >> > > configuration
> > >> >> > >    property.
> > >> >> > >    4. Why would the implementation register Connect resources
> > >> *after*
> > >> >> the
> > >> >> > >    plugins, if Jersey currently registers only the first one?
> The
> > >> >> > "Rejected
> > >> >> > >    Alternatives" mentions why, but this section should be
> > explicit
> > >> >> about
> > >> >> > > why.
> > >> >> > >    For example, "The plugin's would be registered in the
> > >> >> > >    RestServer.start(Herder herder) method before registering
> the
> > >> >> default
> > >> >> > >    Connect resources, which ensures that plugins cannot remove
> > >> Connect
> > >> >> > >    resources."
> > >> >> > >    5. "Hence, it is recommended that the plugins don't
> > re-register
> > >> the
> > >> >> > >    default Connect Resources. This could potentially lead to
> > >> >> unexpected
> > >> >> > >    errors." First, we should not say "recommended" and should
> > just
> > >> say
> > >> >> > > plugins
> > >> >> > >    should not register any resources that conflict with the
> > >> built-in
> > >> >> > > Connect
> > >> >> > >    resources. Second, if the worker does find conflicts, can we
> > >> just
> > >> >> > remove
> > >> >> > >    them before adding the built-in Connect resources?
> > >> >> > >    6. Is it possible for implementations to check whether
> > resources
> > >> >> > already
> > >> >> > >    exist before registering their own? If so, we should
> recommend
> > >> that
> > >> >> > >    implementations do this and log any problems.
> > >> >> > >    7. We should be explicit that the "Service Provider" is
> Java's
> > >> >> Service
> > >> >> > >    Provider API. We also need to be explicit that an
> > implementation
> > >> >> must
> > >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> > >> >> > > ConnectRestPlugin`
> > >> >> > >    file (or whatever the package name of the
> `ConnectRestPlugin`
> > >> will
> > >> >> be)
> > >> >> > > with
> > >> >> > >    the fully-qualified name of the implementation class(es).
> > >> >> > >    8. The example should include the META-INF file required by
> > the
> > >> >> > Service
> > >> >> > >    Provider API.
> > >> >> > >
> > >> >> > > Again, overall this is really great!
> > >> >> > >
> > >> >> > > Best regards,
> > >> >> > >
> > >> >> > > Randall
> > >> >> > >
> > >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > >> >> > mageshn@confluent.io>
> > >> >> > > wrote:
> > >> >> > >
> > >> >> > > > Hi,
> > >> >> > > >
> > >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to add
> > the
> > >> >> > ability
> > >> >> > > to
> > >> >> > > > provide Rest Extensions to Connect Rest API.
> > >> >> > > >
> > >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> > >> >> > > >
> > >> >> > > > Please take a look. Your feedback is appreciated.
> > >> >> > > >
> > >> >> > > > Thanks,
> > >> >> > > >
> > >> >> > > > Magesh
> > >> >> > > >
> > >> >> > >
> > >> >> >
> > >> >>
> > >> >
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Thanks Randall for your thoughts. I have created a replica of the required
entities in the draft implementation. If you can take a look at the PR and
let me know your thoughts, I will update the KIP to reflect the same

https://github.com/apache/kafka/pull/4931

On Tue, Apr 24, 2018 at 11:44 AM, Randall Hauch <rh...@gmail.com> wrote:

> Magesh, I think our last emails cross in mid-stream.
>
> We definitely want to put the new public interfaces/classes in the API
> module, and implementation in the runtime module. Yes, this will affect the
> design, since for example we don't want to expose runtime types to the API,
> and we want to prevent breaking changes. We don't really want to move the
> REST entities if we don't have to, since that may break projects that are
> extending the runtime module -- even though the runtime module is not a
> public API we still want to _try_ to change things.
>
> Do you want to try to create a prototype to see what kind of impact and
> choices we'll have to make?
>
> Best regards,
>
> Randall
>
> On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com> wrote:
>
> > Thanks for updating the KIP, Magesh. You've resolved all of my concerns,
> > though I have one more: we should specify the package names for all new
> > interfaces/classes.
> >
> > I'm looking forward to more feedback from others.
> >
> > Best regards,
> >
> > Randall
> >
> > On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <
> mageshn@confluent.io>
> > wrote:
> >
> >> Hi All,
> >>
> >> I have updated the KIP with following changes
> >>
> >>    1. Expanded the Motivation section
> >>    2. Included details about the interface in the public interface
> section
> >>    3. Modified the config name to rest.extension.classes
> >>    4. Modified the ConnectRestExtension to include Configurable instead
> of
> >>    ResourceConfig
> >>    5. Modified the "Rest Extension Integration with Connect" in
> "Proposed
> >>    Approach" to include a new Custom implementation for Configurable
> >>    6. Provided examples for the Java Service provider mechanism
> >>    7. Included a reference implementation in scope
> >>
> >> Kindly let me know your thoughts on the updates.
> >>
> >> Thanks
> >> Magesh
> >>
> >> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <
> mageshn@confluent.io
> >> >
> >> wrote:
> >>
> >> > Hi Randall,
> >> >
> >> > Thanks for your feedback. I also would like to go with
> >> > rest.extension.classes`.
> >> >
> >> > For exposing Configurable, my original intention was just to expose
> that
> >> > to the extension because that's all one needs to register JAX RS
> >> resources.
> >> > The fact that we use Jersey shouldn't even be exposed in the
> interface.
> >> > Hence it doesn't affect the public API by any means.
> >> >
> >> > I will update the KIP and let everyone know.
> >> >
> >> > Thanks
> >> > Magesh
> >> >
> >> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com>
> >> wrote:
> >> >
> >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> >> mageshn@confluent.io
> >> >> >
> >> >> wrote:
> >> >>
> >> >> > Hi Randall,
> >> >> >
> >> >> > Thanks a lot for your feedback.
> >> >> >
> >> >> > I will update the KIP to reflect your comments in (1), (2), (7) and
> >> (8).
> >> >> >
> >> >>
> >> >> Looking forward to these.
> >> >>
> >> >>
> >> >> >
> >> >> > For comment # (3) , while it would be great to automatically
> >> configure
> >> >> the
> >> >> > Rest Extensions, I would prefer that to be specified explicitly.
> Lets
> >> >> > assume a connector archive includes a implementation for the
> >> >> RestExtension
> >> >> > to do authentication using some header. We don't want this to be
> >> >> > automatically included. Having said that I think that the config
> key
> >> >> name
> >> >> > should probably be changed to something like "rest.extension" or
> >> >> > "rest.extension.class".
> >> >> >
> >> >>
> >> >> That's a good point. I do like `rest.extension.class` (or
> `..classes`?)
> >> >> much more than `rest.plugins`.
> >> >>
> >> >>
> >> >> >
> >> >> > For the comment regarding the resource loading into jersey, I have
> >> the
> >> >> > following proposal
> >> >> >
> >> >> > Create an implementation of Configurable(
> >> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
> >> urable.html
> >> >> )
> >> >> > that delegates to ResourceConfig. In the ConnectRestExtension, we
> >> would
> >> >> > expose only Configurable which is sufficient enough to register new
> >> >> > resources. In the new implementation, we will check if the resource
> >> is
> >> >> > already registered using ResourceConfig.isRegistered() method and
> >> log a
> >> >> > warning if the resource is already registered. This will make it a
> >> >> > deterministic behavior and avoid any potential re-registrations.
> Let
> >> me
> >> >> > know your thoughts on these.
> >> >> >
> >> >>
> >> >> This sounds a good idea. Is it as flexible as the current proposal?
> If
> >> >> not,
> >> >> then I'd love to see how this affects the public APIs.
> >> >>
> >> >>
> >> >> >
> >> >> > Thanks
> >> >> > Magesh
> >> >> >
> >> >> >
> >> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
> >> >> wrote:
> >> >> >
> >> >> > > Very nice proposal, Magesh. I like the approach and the new
> >> concepts
> >> >> and
> >> >> > > interfaces, but I do have a few comments/suggestions about some
> >> >> specific
> >> >> > > details:
> >> >> > >
> >> >> > >    1. In the "Motivation" section, perhaps it makes sense to
> >> briefly
> >> >> > >    describe one or two somewhat concrete examples of how this is
> >> >> useful.
> >> >> > >    2. Maybe in the "Public Interfaces" section you could briefly
> >> >> describe
> >> >> > >    each of the interfaces, what they represent, and which are
> >> >> implemented
> >> >> > > by
> >> >> > >    the framework vs implemented by an extension. I think it'd
> help
> >> to
> >> >> > > explain
> >> >> > >    that only the `ConnectRestPlugin` needs to be implemented, and
> >> the
> >> >> > rest
> >> >> > >    will be provided by the framework. I know the next section
> goes
> >> >> into
> >> >> > it
> >> >> > > a
> >> >> > >    bit, but it'd be useful in this section when first talking
> about
> >> >> the
> >> >> > new
> >> >> > >    interfaces.
> >> >> > >    3. Also in the "Public Interfaces" section: I don't think we
> >> should
> >> >> > >    introduce a "rest.plugins" configuration property. Instead,
> can
> >> we
> >> >> not
> >> >> > > just
> >> >> > >    instantiate and call all of the ConnectRestPlugins that we
> find
> >> on
> >> >> the
> >> >> > >    plugin path? Besides, it seems too close to the `plugin.path`
> >> >> > > configuration
> >> >> > >    property.
> >> >> > >    4. Why would the implementation register Connect resources
> >> *after*
> >> >> the
> >> >> > >    plugins, if Jersey currently registers only the first one? The
> >> >> > "Rejected
> >> >> > >    Alternatives" mentions why, but this section should be
> explicit
> >> >> about
> >> >> > > why.
> >> >> > >    For example, "The plugin's would be registered in the
> >> >> > >    RestServer.start(Herder herder) method before registering the
> >> >> default
> >> >> > >    Connect resources, which ensures that plugins cannot remove
> >> Connect
> >> >> > >    resources."
> >> >> > >    5. "Hence, it is recommended that the plugins don't
> re-register
> >> the
> >> >> > >    default Connect Resources. This could potentially lead to
> >> >> unexpected
> >> >> > >    errors." First, we should not say "recommended" and should
> just
> >> say
> >> >> > > plugins
> >> >> > >    should not register any resources that conflict with the
> >> built-in
> >> >> > > Connect
> >> >> > >    resources. Second, if the worker does find conflicts, can we
> >> just
> >> >> > remove
> >> >> > >    them before adding the built-in Connect resources?
> >> >> > >    6. Is it possible for implementations to check whether
> resources
> >> >> > already
> >> >> > >    exist before registering their own? If so, we should recommend
> >> that
> >> >> > >    implementations do this and log any problems.
> >> >> > >    7. We should be explicit that the "Service Provider" is Java's
> >> >> Service
> >> >> > >    Provider API. We also need to be explicit that an
> implementation
> >> >> must
> >> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> >> >> > > ConnectRestPlugin`
> >> >> > >    file (or whatever the package name of the `ConnectRestPlugin`
> >> will
> >> >> be)
> >> >> > > with
> >> >> > >    the fully-qualified name of the implementation class(es).
> >> >> > >    8. The example should include the META-INF file required by
> the
> >> >> > Service
> >> >> > >    Provider API.
> >> >> > >
> >> >> > > Again, overall this is really great!
> >> >> > >
> >> >> > > Best regards,
> >> >> > >
> >> >> > > Randall
> >> >> > >
> >> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> >> >> > mageshn@confluent.io>
> >> >> > > wrote:
> >> >> > >
> >> >> > > > Hi,
> >> >> > > >
> >> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to add
> the
> >> >> > ability
> >> >> > > to
> >> >> > > > provide Rest Extensions to Connect Rest API.
> >> >> > > >
> >> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> >> >> > > >
> >> >> > > > Please take a look. Your feedback is appreciated.
> >> >> > > >
> >> >> > > > Thanks,
> >> >> > > >
> >> >> > > > Magesh
> >> >> > > >
> >> >> > >
> >> >> >
> >> >>
> >> >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

Magesh, I think our last emails cross in mid-stream.

We definitely want to put the new public interfaces/classes in the API
module, and implementation in the runtime module. Yes, this will affect the
design, since for example we don't want to expose runtime types to the API,
and we want to prevent breaking changes. We don't really want to move the
REST entities if we don't have to, since that may break projects that are
extending the runtime module -- even though the runtime module is not a
public API we still want to _try_ to change things.

Do you want to try to create a prototype to see what kind of impact and
choices we'll have to make?

Best regards,

Randall

On Tue, Apr 24, 2018 at 12:48 PM, Randall Hauch <rh...@gmail.com> wrote:

> Thanks for updating the KIP, Magesh. You've resolved all of my concerns,
> though I have one more: we should specify the package names for all new
> interfaces/classes.
>
> I'm looking forward to more feedback from others.
>
> Best regards,
>
> Randall
>
> On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
>> Hi All,
>>
>> I have updated the KIP with following changes
>>
>>    1. Expanded the Motivation section
>>    2. Included details about the interface in the public interface section
>>    3. Modified the config name to rest.extension.classes
>>    4. Modified the ConnectRestExtension to include Configurable instead of
>>    ResourceConfig
>>    5. Modified the "Rest Extension Integration with Connect" in "Proposed
>>    Approach" to include a new Custom implementation for Configurable
>>    6. Provided examples for the Java Service provider mechanism
>>    7. Included a reference implementation in scope
>>
>> Kindly let me know your thoughts on the updates.
>>
>> Thanks
>> Magesh
>>
>> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <mageshn@confluent.io
>> >
>> wrote:
>>
>> > Hi Randall,
>> >
>> > Thanks for your feedback. I also would like to go with
>> > rest.extension.classes`.
>> >
>> > For exposing Configurable, my original intention was just to expose that
>> > to the extension because that's all one needs to register JAX RS
>> resources.
>> > The fact that we use Jersey shouldn't even be exposed in the interface.
>> > Hence it doesn't affect the public API by any means.
>> >
>> > I will update the KIP and let everyone know.
>> >
>> > Thanks
>> > Magesh
>> >
>> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com>
>> wrote:
>> >
>> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
>> mageshn@confluent.io
>> >> >
>> >> wrote:
>> >>
>> >> > Hi Randall,
>> >> >
>> >> > Thanks a lot for your feedback.
>> >> >
>> >> > I will update the KIP to reflect your comments in (1), (2), (7) and
>> (8).
>> >> >
>> >>
>> >> Looking forward to these.
>> >>
>> >>
>> >> >
>> >> > For comment # (3) , while it would be great to automatically
>> configure
>> >> the
>> >> > Rest Extensions, I would prefer that to be specified explicitly. Lets
>> >> > assume a connector archive includes a implementation for the
>> >> RestExtension
>> >> > to do authentication using some header. We don't want this to be
>> >> > automatically included. Having said that I think that the config key
>> >> name
>> >> > should probably be changed to something like "rest.extension" or
>> >> > "rest.extension.class".
>> >> >
>> >>
>> >> That's a good point. I do like `rest.extension.class` (or `..classes`?)
>> >> much more than `rest.plugins`.
>> >>
>> >>
>> >> >
>> >> > For the comment regarding the resource loading into jersey, I have
>> the
>> >> > following proposal
>> >> >
>> >> > Create an implementation of Configurable(
>> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
>> urable.html
>> >> )
>> >> > that delegates to ResourceConfig. In the ConnectRestExtension, we
>> would
>> >> > expose only Configurable which is sufficient enough to register new
>> >> > resources. In the new implementation, we will check if the resource
>> is
>> >> > already registered using ResourceConfig.isRegistered() method and
>> log a
>> >> > warning if the resource is already registered. This will make it a
>> >> > deterministic behavior and avoid any potential re-registrations. Let
>> me
>> >> > know your thoughts on these.
>> >> >
>> >>
>> >> This sounds a good idea. Is it as flexible as the current proposal? If
>> >> not,
>> >> then I'd love to see how this affects the public APIs.
>> >>
>> >>
>> >> >
>> >> > Thanks
>> >> > Magesh
>> >> >
>> >> >
>> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
>> >> wrote:
>> >> >
>> >> > > Very nice proposal, Magesh. I like the approach and the new
>> concepts
>> >> and
>> >> > > interfaces, but I do have a few comments/suggestions about some
>> >> specific
>> >> > > details:
>> >> > >
>> >> > >    1. In the "Motivation" section, perhaps it makes sense to
>> briefly
>> >> > >    describe one or two somewhat concrete examples of how this is
>> >> useful.
>> >> > >    2. Maybe in the "Public Interfaces" section you could briefly
>> >> describe
>> >> > >    each of the interfaces, what they represent, and which are
>> >> implemented
>> >> > > by
>> >> > >    the framework vs implemented by an extension. I think it'd help
>> to
>> >> > > explain
>> >> > >    that only the `ConnectRestPlugin` needs to be implemented, and
>> the
>> >> > rest
>> >> > >    will be provided by the framework. I know the next section goes
>> >> into
>> >> > it
>> >> > > a
>> >> > >    bit, but it'd be useful in this section when first talking about
>> >> the
>> >> > new
>> >> > >    interfaces.
>> >> > >    3. Also in the "Public Interfaces" section: I don't think we
>> should
>> >> > >    introduce a "rest.plugins" configuration property. Instead, can
>> we
>> >> not
>> >> > > just
>> >> > >    instantiate and call all of the ConnectRestPlugins that we find
>> on
>> >> the
>> >> > >    plugin path? Besides, it seems too close to the `plugin.path`
>> >> > > configuration
>> >> > >    property.
>> >> > >    4. Why would the implementation register Connect resources
>> *after*
>> >> the
>> >> > >    plugins, if Jersey currently registers only the first one? The
>> >> > "Rejected
>> >> > >    Alternatives" mentions why, but this section should be explicit
>> >> about
>> >> > > why.
>> >> > >    For example, "The plugin's would be registered in the
>> >> > >    RestServer.start(Herder herder) method before registering the
>> >> default
>> >> > >    Connect resources, which ensures that plugins cannot remove
>> Connect
>> >> > >    resources."
>> >> > >    5. "Hence, it is recommended that the plugins don't re-register
>> the
>> >> > >    default Connect Resources. This could potentially lead to
>> >> unexpected
>> >> > >    errors." First, we should not say "recommended" and should just
>> say
>> >> > > plugins
>> >> > >    should not register any resources that conflict with the
>> built-in
>> >> > > Connect
>> >> > >    resources. Second, if the worker does find conflicts, can we
>> just
>> >> > remove
>> >> > >    them before adding the built-in Connect resources?
>> >> > >    6. Is it possible for implementations to check whether resources
>> >> > already
>> >> > >    exist before registering their own? If so, we should recommend
>> that
>> >> > >    implementations do this and log any problems.
>> >> > >    7. We should be explicit that the "Service Provider" is Java's
>> >> Service
>> >> > >    Provider API. We also need to be explicit that an implementation
>> >> must
>> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
>> >> > > ConnectRestPlugin`
>> >> > >    file (or whatever the package name of the `ConnectRestPlugin`
>> will
>> >> be)
>> >> > > with
>> >> > >    the fully-qualified name of the implementation class(es).
>> >> > >    8. The example should include the META-INF file required by the
>> >> > Service
>> >> > >    Provider API.
>> >> > >
>> >> > > Again, overall this is really great!
>> >> > >
>> >> > > Best regards,
>> >> > >
>> >> > > Randall
>> >> > >
>> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
>> >> > mageshn@confluent.io>
>> >> > > wrote:
>> >> > >
>> >> > > > Hi,
>> >> > > >
>> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
>> >> > ability
>> >> > > to
>> >> > > > provide Rest Extensions to Connect Rest API.
>> >> > > >
>> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> >> > > > 285%3A+Connect+Rest+Extension+Plugin
>> >> > > >
>> >> > > > Please take a look. Your feedback is appreciated.
>> >> > > >
>> >> > > > Thanks,
>> >> > > >
>> >> > > > Magesh
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>>
>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

Thanks for updating the KIP, Magesh. You've resolved all of my concerns,
though I have one more: we should specify the package names for all new
interfaces/classes.

I'm looking forward to more feedback from others.

Best regards,

Randall

On Fri, Apr 20, 2018 at 12:17 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi All,
>
> I have updated the KIP with following changes
>
>    1. Expanded the Motivation section
>    2. Included details about the interface in the public interface section
>    3. Modified the config name to rest.extension.classes
>    4. Modified the ConnectRestExtension to include Configurable instead of
>    ResourceConfig
>    5. Modified the "Rest Extension Integration with Connect" in "Proposed
>    Approach" to include a new Custom implementation for Configurable
>    6. Provided examples for the Java Service provider mechanism
>    7. Included a reference implementation in scope
>
> Kindly let me know your thoughts on the updates.
>
> Thanks
> Magesh
>
> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi Randall,
> >
> > Thanks for your feedback. I also would like to go with
> > rest.extension.classes`.
> >
> > For exposing Configurable, my original intention was just to expose that
> > to the extension because that's all one needs to register JAX RS
> resources.
> > The fact that we use Jersey shouldn't even be exposed in the interface.
> > Hence it doesn't affect the public API by any means.
> >
> > I will update the KIP and let everyone know.
> >
> > Thanks
> > Magesh
> >
> > On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com> wrote:
> >
> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
> mageshn@confluent.io
> >> >
> >> wrote:
> >>
> >> > Hi Randall,
> >> >
> >> > Thanks a lot for your feedback.
> >> >
> >> > I will update the KIP to reflect your comments in (1), (2), (7) and
> (8).
> >> >
> >>
> >> Looking forward to these.
> >>
> >>
> >> >
> >> > For comment # (3) , while it would be great to automatically configure
> >> the
> >> > Rest Extensions, I would prefer that to be specified explicitly. Lets
> >> > assume a connector archive includes a implementation for the
> >> RestExtension
> >> > to do authentication using some header. We don't want this to be
> >> > automatically included. Having said that I think that the config key
> >> name
> >> > should probably be changed to something like "rest.extension" or
> >> > "rest.extension.class".
> >> >
> >>
> >> That's a good point. I do like `rest.extension.class` (or `..classes`?)
> >> much more than `rest.plugins`.
> >>
> >>
> >> >
> >> > For the comment regarding the resource loading into jersey, I have the
> >> > following proposal
> >> >
> >> > Create an implementation of Configurable(
> >> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/
> Configurable.html
> >> )
> >> > that delegates to ResourceConfig. In the ConnectRestExtension, we
> would
> >> > expose only Configurable which is sufficient enough to register new
> >> > resources. In the new implementation, we will check if the resource is
> >> > already registered using ResourceConfig.isRegistered() method and log
> a
> >> > warning if the resource is already registered. This will make it a
> >> > deterministic behavior and avoid any potential re-registrations. Let
> me
> >> > know your thoughts on these.
> >> >
> >>
> >> This sounds a good idea. Is it as flexible as the current proposal? If
> >> not,
> >> then I'd love to see how this affects the public APIs.
> >>
> >>
> >> >
> >> > Thanks
> >> > Magesh
> >> >
> >> >
> >> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
> >> wrote:
> >> >
> >> > > Very nice proposal, Magesh. I like the approach and the new concepts
> >> and
> >> > > interfaces, but I do have a few comments/suggestions about some
> >> specific
> >> > > details:
> >> > >
> >> > >    1. In the "Motivation" section, perhaps it makes sense to briefly
> >> > >    describe one or two somewhat concrete examples of how this is
> >> useful.
> >> > >    2. Maybe in the "Public Interfaces" section you could briefly
> >> describe
> >> > >    each of the interfaces, what they represent, and which are
> >> implemented
> >> > > by
> >> > >    the framework vs implemented by an extension. I think it'd help
> to
> >> > > explain
> >> > >    that only the `ConnectRestPlugin` needs to be implemented, and
> the
> >> > rest
> >> > >    will be provided by the framework. I know the next section goes
> >> into
> >> > it
> >> > > a
> >> > >    bit, but it'd be useful in this section when first talking about
> >> the
> >> > new
> >> > >    interfaces.
> >> > >    3. Also in the "Public Interfaces" section: I don't think we
> should
> >> > >    introduce a "rest.plugins" configuration property. Instead, can
> we
> >> not
> >> > > just
> >> > >    instantiate and call all of the ConnectRestPlugins that we find
> on
> >> the
> >> > >    plugin path? Besides, it seems too close to the `plugin.path`
> >> > > configuration
> >> > >    property.
> >> > >    4. Why would the implementation register Connect resources
> *after*
> >> the
> >> > >    plugins, if Jersey currently registers only the first one? The
> >> > "Rejected
> >> > >    Alternatives" mentions why, but this section should be explicit
> >> about
> >> > > why.
> >> > >    For example, "The plugin's would be registered in the
> >> > >    RestServer.start(Herder herder) method before registering the
> >> default
> >> > >    Connect resources, which ensures that plugins cannot remove
> Connect
> >> > >    resources."
> >> > >    5. "Hence, it is recommended that the plugins don't re-register
> the
> >> > >    default Connect Resources. This could potentially lead to
> >> unexpected
> >> > >    errors." First, we should not say "recommended" and should just
> say
> >> > > plugins
> >> > >    should not register any resources that conflict with the built-in
> >> > > Connect
> >> > >    resources. Second, if the worker does find conflicts, can we just
> >> > remove
> >> > >    them before adding the built-in Connect resources?
> >> > >    6. Is it possible for implementations to check whether resources
> >> > already
> >> > >    exist before registering their own? If so, we should recommend
> that
> >> > >    implementations do this and log any problems.
> >> > >    7. We should be explicit that the "Service Provider" is Java's
> >> Service
> >> > >    Provider API. We also need to be explicit that an implementation
> >> must
> >> > >    provide a `META-INF/services/org.apache.kafka.connect.
> >> > > ConnectRestPlugin`
> >> > >    file (or whatever the package name of the `ConnectRestPlugin`
> will
> >> be)
> >> > > with
> >> > >    the fully-qualified name of the implementation class(es).
> >> > >    8. The example should include the META-INF file required by the
> >> > Service
> >> > >    Provider API.
> >> > >
> >> > > Again, overall this is really great!
> >> > >
> >> > > Best regards,
> >> > >
> >> > > Randall
> >> > >
> >> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> >> > mageshn@confluent.io>
> >> > > wrote:
> >> > >
> >> > > > Hi,
> >> > > >
> >> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
> >> > ability
> >> > > to
> >> > > > provide Rest Extensions to Connect Rest API.
> >> > > >
> >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > > > 285%3A+Connect+Rest+Extension+Plugin
> >> > > >
> >> > > > Please take a look. Your feedback is appreciated.
> >> > > >
> >> > > > Thanks,
> >> > > >
> >> > > > Magesh
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Randall,

I'm trying to put together a PR for this KIP with the understanding that that
all public interfaces should live in the api module and in the process I
noticed the following


   1. WorkerConfig is available in runtime. If we are exposing it to the
   Extension, do we just move it to the API module?
   2. All of the rest entities today live in  runtime. We are
   exposing org.apache.kafka.connect.runtime.rest.entities.ConnectorStateInfo
   via our plugin interface. So , was wondering if these entities can move to
   API since they are already in a way public.


If we don't want to do that all the new interfaces would need to live in
the runtime. This might affect some minor details in the KIP. Let me know
your thoughts.

Thanks
Magesh


On Thu, Apr 19, 2018 at 10:17 PM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi All,
>
> I have updated the KIP with following changes
>
>    1. Expanded the Motivation section
>    2. Included details about the interface in the public interface section
>    3. Modified the config name to rest.extension.classes
>    4. Modified the ConnectRestExtension to include Configurable instead
>    of ResourceConfig
>    5. Modified the "Rest Extension Integration with Connect" in "Proposed
>    Approach" to include a new Custom implementation for Configurable
>    6. Provided examples for the Java Service provider mechanism
>    7. Included a reference implementation in scope
>
> Kindly let me know your thoughts on the updates.
>
> Thanks
> Magesh
>
> On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
>> Hi Randall,
>>
>> Thanks for your feedback. I also would like to go with
>> rest.extension.classes`.
>>
>> For exposing Configurable, my original intention was just to expose that
>> to the extension because that's all one needs to register JAX RS resources.
>> The fact that we use Jersey shouldn't even be exposed in the interface.
>> Hence it doesn't affect the public API by any means.
>>
>> I will update the KIP and let everyone know.
>>
>> Thanks
>> Magesh
>>
>> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com> wrote:
>>
>>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <
>>> mageshn@confluent.io>
>>> wrote:
>>>
>>> > Hi Randall,
>>> >
>>> > Thanks a lot for your feedback.
>>> >
>>> > I will update the KIP to reflect your comments in (1), (2), (7) and
>>> (8).
>>> >
>>>
>>> Looking forward to these.
>>>
>>>
>>> >
>>> > For comment # (3) , while it would be great to automatically configure
>>> the
>>> > Rest Extensions, I would prefer that to be specified explicitly. Lets
>>> > assume a connector archive includes a implementation for the
>>> RestExtension
>>> > to do authentication using some header. We don't want this to be
>>> > automatically included. Having said that I think that the config key
>>> name
>>> > should probably be changed to something like "rest.extension" or
>>> > "rest.extension.class".
>>> >
>>>
>>> That's a good point. I do like `rest.extension.class` (or `..classes`?)
>>> much more than `rest.plugins`.
>>>
>>>
>>> >
>>> > For the comment regarding the resource loading into jersey, I have the
>>> > following proposal
>>> >
>>> > Create an implementation of Configurable(
>>> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Config
>>> urable.html)
>>> > that delegates to ResourceConfig. In the ConnectRestExtension, we would
>>> > expose only Configurable which is sufficient enough to register new
>>> > resources. In the new implementation, we will check if the resource is
>>> > already registered using ResourceConfig.isRegistered() method and log a
>>> > warning if the resource is already registered. This will make it a
>>> > deterministic behavior and avoid any potential re-registrations. Let me
>>> > know your thoughts on these.
>>> >
>>>
>>> This sounds a good idea. Is it as flexible as the current proposal? If
>>> not,
>>> then I'd love to see how this affects the public APIs.
>>>
>>>
>>> >
>>> > Thanks
>>> > Magesh
>>> >
>>> >
>>> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
>>> wrote:
>>> >
>>> > > Very nice proposal, Magesh. I like the approach and the new concepts
>>> and
>>> > > interfaces, but I do have a few comments/suggestions about some
>>> specific
>>> > > details:
>>> > >
>>> > >    1. In the "Motivation" section, perhaps it makes sense to briefly
>>> > >    describe one or two somewhat concrete examples of how this is
>>> useful.
>>> > >    2. Maybe in the "Public Interfaces" section you could briefly
>>> describe
>>> > >    each of the interfaces, what they represent, and which are
>>> implemented
>>> > > by
>>> > >    the framework vs implemented by an extension. I think it'd help to
>>> > > explain
>>> > >    that only the `ConnectRestPlugin` needs to be implemented, and the
>>> > rest
>>> > >    will be provided by the framework. I know the next section goes
>>> into
>>> > it
>>> > > a
>>> > >    bit, but it'd be useful in this section when first talking about
>>> the
>>> > new
>>> > >    interfaces.
>>> > >    3. Also in the "Public Interfaces" section: I don't think we
>>> should
>>> > >    introduce a "rest.plugins" configuration property. Instead, can
>>> we not
>>> > > just
>>> > >    instantiate and call all of the ConnectRestPlugins that we find
>>> on the
>>> > >    plugin path? Besides, it seems too close to the `plugin.path`
>>> > > configuration
>>> > >    property.
>>> > >    4. Why would the implementation register Connect resources
>>> *after* the
>>> > >    plugins, if Jersey currently registers only the first one? The
>>> > "Rejected
>>> > >    Alternatives" mentions why, but this section should be explicit
>>> about
>>> > > why.
>>> > >    For example, "The plugin's would be registered in the
>>> > >    RestServer.start(Herder herder) method before registering the
>>> default
>>> > >    Connect resources, which ensures that plugins cannot remove
>>> Connect
>>> > >    resources."
>>> > >    5. "Hence, it is recommended that the plugins don't re-register
>>> the
>>> > >    default Connect Resources. This could potentially lead to
>>> unexpected
>>> > >    errors." First, we should not say "recommended" and should just
>>> say
>>> > > plugins
>>> > >    should not register any resources that conflict with the built-in
>>> > > Connect
>>> > >    resources. Second, if the worker does find conflicts, can we just
>>> > remove
>>> > >    them before adding the built-in Connect resources?
>>> > >    6. Is it possible for implementations to check whether resources
>>> > already
>>> > >    exist before registering their own? If so, we should recommend
>>> that
>>> > >    implementations do this and log any problems.
>>> > >    7. We should be explicit that the "Service Provider" is Java's
>>> Service
>>> > >    Provider API. We also need to be explicit that an implementation
>>> must
>>> > >    provide a `META-INF/services/org.apache.kafka.connect.
>>> > > ConnectRestPlugin`
>>> > >    file (or whatever the package name of the `ConnectRestPlugin`
>>> will be)
>>> > > with
>>> > >    the fully-qualified name of the implementation class(es).
>>> > >    8. The example should include the META-INF file required by the
>>> > Service
>>> > >    Provider API.
>>> > >
>>> > > Again, overall this is really great!
>>> > >
>>> > > Best regards,
>>> > >
>>> > > Randall
>>> > >
>>> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
>>> > mageshn@confluent.io>
>>> > > wrote:
>>> > >
>>> > > > Hi,
>>> > > >
>>> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
>>> > ability
>>> > > to
>>> > > > provide Rest Extensions to Connect Rest API.
>>> > > >
>>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>> > > > 285%3A+Connect+Rest+Extension+Plugin
>>> > > >
>>> > > > Please take a look. Your feedback is appreciated.
>>> > > >
>>> > > > Thanks,
>>> > > >
>>> > > > Magesh
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi All,

I have updated the KIP with following changes

   1. Expanded the Motivation section
   2. Included details about the interface in the public interface section
   3. Modified the config name to rest.extension.classes
   4. Modified the ConnectRestExtension to include Configurable instead of
   ResourceConfig
   5. Modified the "Rest Extension Integration with Connect" in "Proposed
   Approach" to include a new Custom implementation for Configurable
   6. Provided examples for the Java Service provider mechanism
   7. Included a reference implementation in scope

Kindly let me know your thoughts on the updates.

Thanks
Magesh

On Thu, Apr 19, 2018 at 10:39 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi Randall,
>
> Thanks for your feedback. I also would like to go with
> rest.extension.classes`.
>
> For exposing Configurable, my original intention was just to expose that
> to the extension because that's all one needs to register JAX RS resources.
> The fact that we use Jersey shouldn't even be exposed in the interface.
> Hence it doesn't affect the public API by any means.
>
> I will update the KIP and let everyone know.
>
> Thanks
> Magesh
>
> On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com> wrote:
>
>> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <mageshn@confluent.io
>> >
>> wrote:
>>
>> > Hi Randall,
>> >
>> > Thanks a lot for your feedback.
>> >
>> > I will update the KIP to reflect your comments in (1), (2), (7) and (8).
>> >
>>
>> Looking forward to these.
>>
>>
>> >
>> > For comment # (3) , while it would be great to automatically configure
>> the
>> > Rest Extensions, I would prefer that to be specified explicitly. Lets
>> > assume a connector archive includes a implementation for the
>> RestExtension
>> > to do authentication using some header. We don't want this to be
>> > automatically included. Having said that I think that the config key
>> name
>> > should probably be changed to something like "rest.extension" or
>> > "rest.extension.class".
>> >
>>
>> That's a good point. I do like `rest.extension.class` (or `..classes`?)
>> much more than `rest.plugins`.
>>
>>
>> >
>> > For the comment regarding the resource loading into jersey, I have the
>> > following proposal
>> >
>> > Create an implementation of Configurable(
>> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Configurable.html
>> )
>> > that delegates to ResourceConfig. In the ConnectRestExtension, we would
>> > expose only Configurable which is sufficient enough to register new
>> > resources. In the new implementation, we will check if the resource is
>> > already registered using ResourceConfig.isRegistered() method and log a
>> > warning if the resource is already registered. This will make it a
>> > deterministic behavior and avoid any potential re-registrations. Let me
>> > know your thoughts on these.
>> >
>>
>> This sounds a good idea. Is it as flexible as the current proposal? If
>> not,
>> then I'd love to see how this affects the public APIs.
>>
>>
>> >
>> > Thanks
>> > Magesh
>> >
>> >
>> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
>> wrote:
>> >
>> > > Very nice proposal, Magesh. I like the approach and the new concepts
>> and
>> > > interfaces, but I do have a few comments/suggestions about some
>> specific
>> > > details:
>> > >
>> > >    1. In the "Motivation" section, perhaps it makes sense to briefly
>> > >    describe one or two somewhat concrete examples of how this is
>> useful.
>> > >    2. Maybe in the "Public Interfaces" section you could briefly
>> describe
>> > >    each of the interfaces, what they represent, and which are
>> implemented
>> > > by
>> > >    the framework vs implemented by an extension. I think it'd help to
>> > > explain
>> > >    that only the `ConnectRestPlugin` needs to be implemented, and the
>> > rest
>> > >    will be provided by the framework. I know the next section goes
>> into
>> > it
>> > > a
>> > >    bit, but it'd be useful in this section when first talking about
>> the
>> > new
>> > >    interfaces.
>> > >    3. Also in the "Public Interfaces" section: I don't think we should
>> > >    introduce a "rest.plugins" configuration property. Instead, can we
>> not
>> > > just
>> > >    instantiate and call all of the ConnectRestPlugins that we find on
>> the
>> > >    plugin path? Besides, it seems too close to the `plugin.path`
>> > > configuration
>> > >    property.
>> > >    4. Why would the implementation register Connect resources *after*
>> the
>> > >    plugins, if Jersey currently registers only the first one? The
>> > "Rejected
>> > >    Alternatives" mentions why, but this section should be explicit
>> about
>> > > why.
>> > >    For example, "The plugin's would be registered in the
>> > >    RestServer.start(Herder herder) method before registering the
>> default
>> > >    Connect resources, which ensures that plugins cannot remove Connect
>> > >    resources."
>> > >    5. "Hence, it is recommended that the plugins don't re-register the
>> > >    default Connect Resources. This could potentially lead to
>> unexpected
>> > >    errors." First, we should not say "recommended" and should just say
>> > > plugins
>> > >    should not register any resources that conflict with the built-in
>> > > Connect
>> > >    resources. Second, if the worker does find conflicts, can we just
>> > remove
>> > >    them before adding the built-in Connect resources?
>> > >    6. Is it possible for implementations to check whether resources
>> > already
>> > >    exist before registering their own? If so, we should recommend that
>> > >    implementations do this and log any problems.
>> > >    7. We should be explicit that the "Service Provider" is Java's
>> Service
>> > >    Provider API. We also need to be explicit that an implementation
>> must
>> > >    provide a `META-INF/services/org.apache.kafka.connect.
>> > > ConnectRestPlugin`
>> > >    file (or whatever the package name of the `ConnectRestPlugin` will
>> be)
>> > > with
>> > >    the fully-qualified name of the implementation class(es).
>> > >    8. The example should include the META-INF file required by the
>> > Service
>> > >    Provider API.
>> > >
>> > > Again, overall this is really great!
>> > >
>> > > Best regards,
>> > >
>> > > Randall
>> > >
>> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
>> > mageshn@confluent.io>
>> > > wrote:
>> > >
>> > > > Hi,
>> > > >
>> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
>> > ability
>> > > to
>> > > > provide Rest Extensions to Connect Rest API.
>> > > >
>> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > > > 285%3A+Connect+Rest+Extension+Plugin
>> > > >
>> > > > Please take a look. Your feedback is appreciated.
>> > > >
>> > > > Thanks,
>> > > >
>> > > > Magesh
>> > > >
>> > >
>> >
>>
>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi Randall,

Thanks for your feedback. I also would like to go with
rest.extension.classes`.

For exposing Configurable, my original intention was just to expose that to
the extension because that's all one needs to register JAX RS resources.
The fact that we use Jersey shouldn't even be exposed in the interface.
Hence it doesn't affect the public API by any means.

I will update the KIP and let everyone know.

Thanks
Magesh

On Thu, Apr 19, 2018 at 9:51 AM, Randall Hauch <rh...@gmail.com> wrote:

> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi Randall,
> >
> > Thanks a lot for your feedback.
> >
> > I will update the KIP to reflect your comments in (1), (2), (7) and (8).
> >
>
> Looking forward to these.
>
>
> >
> > For comment # (3) , while it would be great to automatically configure
> the
> > Rest Extensions, I would prefer that to be specified explicitly. Lets
> > assume a connector archive includes a implementation for the
> RestExtension
> > to do authentication using some header. We don't want this to be
> > automatically included. Having said that I think that the config key name
> > should probably be changed to something like "rest.extension" or
> > "rest.extension.class".
> >
>
> That's a good point. I do like `rest.extension.class` (or `..classes`?)
> much more than `rest.plugins`.
>
>
> >
> > For the comment regarding the resource loading into jersey, I have the
> > following proposal
> >
> > Create an implementation of Configurable(
> > https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Configurable.html)
> > that delegates to ResourceConfig. In the ConnectRestExtension, we would
> > expose only Configurable which is sufficient enough to register new
> > resources. In the new implementation, we will check if the resource is
> > already registered using ResourceConfig.isRegistered() method and log a
> > warning if the resource is already registered. This will make it a
> > deterministic behavior and avoid any potential re-registrations. Let me
> > know your thoughts on these.
> >
>
> This sounds a good idea. Is it as flexible as the current proposal? If not,
> then I'd love to see how this affects the public APIs.
>
>
> >
> > Thanks
> > Magesh
> >
> >
> > On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com>
> wrote:
> >
> > > Very nice proposal, Magesh. I like the approach and the new concepts
> and
> > > interfaces, but I do have a few comments/suggestions about some
> specific
> > > details:
> > >
> > >    1. In the "Motivation" section, perhaps it makes sense to briefly
> > >    describe one or two somewhat concrete examples of how this is
> useful.
> > >    2. Maybe in the "Public Interfaces" section you could briefly
> describe
> > >    each of the interfaces, what they represent, and which are
> implemented
> > > by
> > >    the framework vs implemented by an extension. I think it'd help to
> > > explain
> > >    that only the `ConnectRestPlugin` needs to be implemented, and the
> > rest
> > >    will be provided by the framework. I know the next section goes into
> > it
> > > a
> > >    bit, but it'd be useful in this section when first talking about the
> > new
> > >    interfaces.
> > >    3. Also in the "Public Interfaces" section: I don't think we should
> > >    introduce a "rest.plugins" configuration property. Instead, can we
> not
> > > just
> > >    instantiate and call all of the ConnectRestPlugins that we find on
> the
> > >    plugin path? Besides, it seems too close to the `plugin.path`
> > > configuration
> > >    property.
> > >    4. Why would the implementation register Connect resources *after*
> the
> > >    plugins, if Jersey currently registers only the first one? The
> > "Rejected
> > >    Alternatives" mentions why, but this section should be explicit
> about
> > > why.
> > >    For example, "The plugin's would be registered in the
> > >    RestServer.start(Herder herder) method before registering the
> default
> > >    Connect resources, which ensures that plugins cannot remove Connect
> > >    resources."
> > >    5. "Hence, it is recommended that the plugins don't re-register the
> > >    default Connect Resources. This could potentially lead to unexpected
> > >    errors." First, we should not say "recommended" and should just say
> > > plugins
> > >    should not register any resources that conflict with the built-in
> > > Connect
> > >    resources. Second, if the worker does find conflicts, can we just
> > remove
> > >    them before adding the built-in Connect resources?
> > >    6. Is it possible for implementations to check whether resources
> > already
> > >    exist before registering their own? If so, we should recommend that
> > >    implementations do this and log any problems.
> > >    7. We should be explicit that the "Service Provider" is Java's
> Service
> > >    Provider API. We also need to be explicit that an implementation
> must
> > >    provide a `META-INF/services/org.apache.kafka.connect.
> > > ConnectRestPlugin`
> > >    file (or whatever the package name of the `ConnectRestPlugin` will
> be)
> > > with
> > >    the fully-qualified name of the implementation class(es).
> > >    8. The example should include the META-INF file required by the
> > Service
> > >    Provider API.
> > >
> > > Again, overall this is really great!
> > >
> > > Best regards,
> > >
> > > Randall
> > >
> > > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
> > ability
> > > to
> > > > provide Rest Extensions to Connect Rest API.
> > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 285%3A+Connect+Rest+Extension+Plugin
> > > >
> > > > Please take a look. Your feedback is appreciated.
> > > >
> > > > Thanks,
> > > >
> > > > Magesh
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi Randall,
>
> Thanks a lot for your feedback.
>
> I will update the KIP to reflect your comments in (1), (2), (7) and (8).
>

Looking forward to these.


>
> For comment # (3) , while it would be great to automatically configure the
> Rest Extensions, I would prefer that to be specified explicitly. Lets
> assume a connector archive includes a implementation for the RestExtension
> to do authentication using some header. We don't want this to be
> automatically included. Having said that I think that the config key name
> should probably be changed to something like "rest.extension" or
> "rest.extension.class".
>

That's a good point. I do like `rest.extension.class` (or `..classes`?)
much more than `rest.plugins`.


>
> For the comment regarding the resource loading into jersey, I have the
> following proposal
>
> Create an implementation of Configurable(
> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Configurable.html)
> that delegates to ResourceConfig. In the ConnectRestExtension, we would
> expose only Configurable which is sufficient enough to register new
> resources. In the new implementation, we will check if the resource is
> already registered using ResourceConfig.isRegistered() method and log a
> warning if the resource is already registered. This will make it a
> deterministic behavior and avoid any potential re-registrations. Let me
> know your thoughts on these.
>

This sounds a good idea. Is it as flexible as the current proposal? If not,
then I'd love to see how this affects the public APIs.


>
> Thanks
> Magesh
>
>
> On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com> wrote:
>
> > Very nice proposal, Magesh. I like the approach and the new concepts and
> > interfaces, but I do have a few comments/suggestions about some specific
> > details:
> >
> >    1. In the "Motivation" section, perhaps it makes sense to briefly
> >    describe one or two somewhat concrete examples of how this is useful.
> >    2. Maybe in the "Public Interfaces" section you could briefly describe
> >    each of the interfaces, what they represent, and which are implemented
> > by
> >    the framework vs implemented by an extension. I think it'd help to
> > explain
> >    that only the `ConnectRestPlugin` needs to be implemented, and the
> rest
> >    will be provided by the framework. I know the next section goes into
> it
> > a
> >    bit, but it'd be useful in this section when first talking about the
> new
> >    interfaces.
> >    3. Also in the "Public Interfaces" section: I don't think we should
> >    introduce a "rest.plugins" configuration property. Instead, can we not
> > just
> >    instantiate and call all of the ConnectRestPlugins that we find on the
> >    plugin path? Besides, it seems too close to the `plugin.path`
> > configuration
> >    property.
> >    4. Why would the implementation register Connect resources *after* the
> >    plugins, if Jersey currently registers only the first one? The
> "Rejected
> >    Alternatives" mentions why, but this section should be explicit about
> > why.
> >    For example, "The plugin's would be registered in the
> >    RestServer.start(Herder herder) method before registering the default
> >    Connect resources, which ensures that plugins cannot remove Connect
> >    resources."
> >    5. "Hence, it is recommended that the plugins don't re-register the
> >    default Connect Resources. This could potentially lead to unexpected
> >    errors." First, we should not say "recommended" and should just say
> > plugins
> >    should not register any resources that conflict with the built-in
> > Connect
> >    resources. Second, if the worker does find conflicts, can we just
> remove
> >    them before adding the built-in Connect resources?
> >    6. Is it possible for implementations to check whether resources
> already
> >    exist before registering their own? If so, we should recommend that
> >    implementations do this and log any problems.
> >    7. We should be explicit that the "Service Provider" is Java's Service
> >    Provider API. We also need to be explicit that an implementation must
> >    provide a `META-INF/services/org.apache.kafka.connect.
> > ConnectRestPlugin`
> >    file (or whatever the package name of the `ConnectRestPlugin` will be)
> > with
> >    the fully-qualified name of the implementation class(es).
> >    8. The example should include the META-INF file required by the
> Service
> >    Provider API.
> >
> > Again, overall this is really great!
> >
> > Best regards,
> >
> > Randall
> >
> > On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <
> mageshn@confluent.io>
> > wrote:
> >
> > > Hi,
> > >
> > > We have posted KIP-285: Connect Rest Extension Plugin to add the
> ability
> > to
> > > provide Rest Extensions to Connect Rest API.
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 285%3A+Connect+Rest+Extension+Plugin
> > >
> > > Please take a look. Your feedback is appreciated.
> > >
> > > Thanks,
> > >
> > > Magesh
> > >
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi Randall,

Thanks a lot for your feedback.

I will update the KIP to reflect your comments in (1), (2), (7) and (8).

For comment # (3) , while it would be great to automatically configure the
Rest Extensions, I would prefer that to be specified explicitly. Lets
assume a connector archive includes a implementation for the RestExtension
to do authentication using some header. We don't want this to be
automatically included. Having said that I think that the config key name
should probably be changed to something like "rest.extension" or
"rest.extension.class".

For the comment regarding the resource loading into jersey, I have the
following proposal

Create an implementation of Configurable(
https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Configurable.html)
that delegates to ResourceConfig. In the ConnectRestExtension, we would
expose only Configurable which is sufficient enough to register new
resources. In the new implementation, we will check if the resource is
already registered using ResourceConfig.isRegistered() method and log a
warning if the resource is already registered. This will make it a
deterministic behavior and avoid any potential re-registrations. Let me
know your thoughts on these.

Thanks
Magesh


On Wed, Apr 11, 2018 at 10:06 AM, Randall Hauch <rh...@gmail.com> wrote:

> Very nice proposal, Magesh. I like the approach and the new concepts and
> interfaces, but I do have a few comments/suggestions about some specific
> details:
>
>    1. In the "Motivation" section, perhaps it makes sense to briefly
>    describe one or two somewhat concrete examples of how this is useful.
>    2. Maybe in the "Public Interfaces" section you could briefly describe
>    each of the interfaces, what they represent, and which are implemented
> by
>    the framework vs implemented by an extension. I think it'd help to
> explain
>    that only the `ConnectRestPlugin` needs to be implemented, and the rest
>    will be provided by the framework. I know the next section goes into it
> a
>    bit, but it'd be useful in this section when first talking about the new
>    interfaces.
>    3. Also in the "Public Interfaces" section: I don't think we should
>    introduce a "rest.plugins" configuration property. Instead, can we not
> just
>    instantiate and call all of the ConnectRestPlugins that we find on the
>    plugin path? Besides, it seems too close to the `plugin.path`
> configuration
>    property.
>    4. Why would the implementation register Connect resources *after* the
>    plugins, if Jersey currently registers only the first one? The "Rejected
>    Alternatives" mentions why, but this section should be explicit about
> why.
>    For example, "The plugin's would be registered in the
>    RestServer.start(Herder herder) method before registering the default
>    Connect resources, which ensures that plugins cannot remove Connect
>    resources."
>    5. "Hence, it is recommended that the plugins don't re-register the
>    default Connect Resources. This could potentially lead to unexpected
>    errors." First, we should not say "recommended" and should just say
> plugins
>    should not register any resources that conflict with the built-in
> Connect
>    resources. Second, if the worker does find conflicts, can we just remove
>    them before adding the built-in Connect resources?
>    6. Is it possible for implementations to check whether resources already
>    exist before registering their own? If so, we should recommend that
>    implementations do this and log any problems.
>    7. We should be explicit that the "Service Provider" is Java's Service
>    Provider API. We also need to be explicit that an implementation must
>    provide a `META-INF/services/org.apache.kafka.connect.
> ConnectRestPlugin`
>    file (or whatever the package name of the `ConnectRestPlugin` will be)
> with
>    the fully-qualified name of the implementation class(es).
>    8. The example should include the META-INF file required by the Service
>    Provider API.
>
> Again, overall this is really great!
>
> Best regards,
>
> Randall
>
> On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi,
> >
> > We have posted KIP-285: Connect Rest Extension Plugin to add the ability
> to
> > provide Rest Extensions to Connect Rest API.
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 285%3A+Connect+Rest+Extension+Plugin
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> >
> > Magesh
> >
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Randall Hauch <rh...@gmail.com>]

Very nice proposal, Magesh. I like the approach and the new concepts and
interfaces, but I do have a few comments/suggestions about some specific
details:

   1. In the "Motivation" section, perhaps it makes sense to briefly
   describe one or two somewhat concrete examples of how this is useful.
   2. Maybe in the "Public Interfaces" section you could briefly describe
   each of the interfaces, what they represent, and which are implemented by
   the framework vs implemented by an extension. I think it'd help to explain
   that only the `ConnectRestPlugin` needs to be implemented, and the rest
   will be provided by the framework. I know the next section goes into it a
   bit, but it'd be useful in this section when first talking about the new
   interfaces.
   3. Also in the "Public Interfaces" section: I don't think we should
   introduce a "rest.plugins" configuration property. Instead, can we not just
   instantiate and call all of the ConnectRestPlugins that we find on the
   plugin path? Besides, it seems too close to the `plugin.path` configuration
   property.
   4. Why would the implementation register Connect resources *after* the
   plugins, if Jersey currently registers only the first one? The "Rejected
   Alternatives" mentions why, but this section should be explicit about why.
   For example, "The plugin's would be registered in the
   RestServer.start(Herder herder) method before registering the default
   Connect resources, which ensures that plugins cannot remove Connect
   resources."
   5. "Hence, it is recommended that the plugins don't re-register the
   default Connect Resources. This could potentially lead to unexpected
   errors." First, we should not say "recommended" and should just say plugins
   should not register any resources that conflict with the built-in Connect
   resources. Second, if the worker does find conflicts, can we just remove
   them before adding the built-in Connect resources?
   6. Is it possible for implementations to check whether resources already
   exist before registering their own? If so, we should recommend that
   implementations do this and log any problems.
   7. We should be explicit that the "Service Provider" is Java's Service
   Provider API. We also need to be explicit that an implementation must
   provide a `META-INF/services/org.apache.kafka.connect.ConnectRestPlugin`
   file (or whatever the package name of the `ConnectRestPlugin` will be) with
   the fully-qualified name of the implementation class(es).
   8. The example should include the META-INF file required by the Service
   Provider API.

Again, overall this is really great!

Best regards,

Randall

On Wed, Apr 11, 2018 at 12:14 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi,
>
> We have posted KIP-285: Connect Rest Extension Plugin to add the ability to
> provide Rest Extensions to Connect Rest API.
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 285%3A+Connect+Rest+Extension+Plugin
>
> Please take a look. Your feedback is appreciated.
>
> Thanks,
>
> Magesh
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Sounds Good. I will update the KIP to include an extension that
authenticates Basic Auth Credentials against a properties file. Let me know
if that works.

Thanks
Magesh

On Wed, Apr 11, 2018 at 9:51 AM, Gwen Shapira <gw...@confluent.io> wrote:

> Not a blocker, but it will continue a good tradition to add something
> simple that works. I'm thinking plain-text auth and maybe ZK storage (like
> the Kafka authorizer). It doesn't have to provide real security in any
> sense, but it will allow people to test and experiment. Otherwise, if I
> write an implementation and it doesn't work, it will be harder to know if
> it is me or Kafka...
>
> On Wed, Apr 11, 2018 at 9:47 AM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi Gwen,
> >
> > Thanks for your feedback. As part of this KIP, I was planning on adding a
> > skeleton implementation in the examples but that can be used only as a
> > reference. At this moment, there is no plan to add a concrete
> > implementation of the plugin. Let me know your thoughts :).
> >
> > Thanks,
> > Magesh
> >
> > On Wed, Apr 11, 2018 at 9:42 AM, Gwen Shapira <gw...@confluent.io> wrote:
> >
> > > Thanks Magesh! The lack of authentication and authorization in the REST
> > API
> > > is definitely a painpoint and something that can prevent a company from
> > > adopting connect (or surviving an audit after they did!).
> > >
> > > One thing that isn't clear to me from the KIP: In the past when we
> > > contributed pluggable interfaces, we always contributed a simple
> > > implementation that can be used for testing, experimentation and as an
> > > example to future implementers. The JSON converter is one such example.
> > Or
> > > the ZK authorizer.
> > >
> > > Do you plan on including a simple implementation as part of this KIP as
> > > well?
> > >
> > > Gwen
> > >
> > > On Tue, Apr 10, 2018 at 10:14 PM, Magesh Nandakumar <
> > mageshn@confluent.io>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > We have posted KIP-285: Connect Rest Extension Plugin to add the
> > ability
> > > to
> > > > provide Rest Extensions to Connect Rest API.
> > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 285%3A+Connect+Rest+Extension+Plugin
> > > >
> > > > Please take a look. Your feedback is appreciated.
> > > >
> > > > Thanks,
> > > >
> > > > Magesh
> > > >
> > >
> > >
> > >
> > > --
> > > *Gwen Shapira*
> > > Product Manager | Confluent
> > > 650.450.2760 | @gwenshap
> > > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> > > <http://www.confluent.io/blog>
> > >
> >
>
>
>
> --
> *Gwen Shapira*
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> <http://www.confluent.io/blog>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Gwen Shapira <gw...@confluent.io>]

Not a blocker, but it will continue a good tradition to add something
simple that works. I'm thinking plain-text auth and maybe ZK storage (like
the Kafka authorizer). It doesn't have to provide real security in any
sense, but it will allow people to test and experiment. Otherwise, if I
write an implementation and it doesn't work, it will be harder to know if
it is me or Kafka...

On Wed, Apr 11, 2018 at 9:47 AM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi Gwen,
>
> Thanks for your feedback. As part of this KIP, I was planning on adding a
> skeleton implementation in the examples but that can be used only as a
> reference. At this moment, there is no plan to add a concrete
> implementation of the plugin. Let me know your thoughts :).
>
> Thanks,
> Magesh
>
> On Wed, Apr 11, 2018 at 9:42 AM, Gwen Shapira <gw...@confluent.io> wrote:
>
> > Thanks Magesh! The lack of authentication and authorization in the REST
> API
> > is definitely a painpoint and something that can prevent a company from
> > adopting connect (or surviving an audit after they did!).
> >
> > One thing that isn't clear to me from the KIP: In the past when we
> > contributed pluggable interfaces, we always contributed a simple
> > implementation that can be used for testing, experimentation and as an
> > example to future implementers. The JSON converter is one such example.
> Or
> > the ZK authorizer.
> >
> > Do you plan on including a simple implementation as part of this KIP as
> > well?
> >
> > Gwen
> >
> > On Tue, Apr 10, 2018 at 10:14 PM, Magesh Nandakumar <
> mageshn@confluent.io>
> > wrote:
> >
> > > Hi,
> > >
> > > We have posted KIP-285: Connect Rest Extension Plugin to add the
> ability
> > to
> > > provide Rest Extensions to Connect Rest API.
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 285%3A+Connect+Rest+Extension+Plugin
> > >
> > > Please take a look. Your feedback is appreciated.
> > >
> > > Thanks,
> > >
> > > Magesh
> > >
> >
> >
> >
> > --
> > *Gwen Shapira*
> > Product Manager | Confluent
> > 650.450.2760 | @gwenshap
> > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> > <http://www.confluent.io/blog>
> >
>



-- 
*Gwen Shapira*
Product Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
<http://www.confluent.io/blog>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar <ma...@confluent.io>]

Hi Gwen,

Thanks for your feedback. As part of this KIP, I was planning on adding a
skeleton implementation in the examples but that can be used only as a
reference. At this moment, there is no plan to add a concrete
implementation of the plugin. Let me know your thoughts :).

Thanks,
Magesh

On Wed, Apr 11, 2018 at 9:42 AM, Gwen Shapira <gw...@confluent.io> wrote:

> Thanks Magesh! The lack of authentication and authorization in the REST API
> is definitely a painpoint and something that can prevent a company from
> adopting connect (or surviving an audit after they did!).
>
> One thing that isn't clear to me from the KIP: In the past when we
> contributed pluggable interfaces, we always contributed a simple
> implementation that can be used for testing, experimentation and as an
> example to future implementers. The JSON converter is one such example. Or
> the ZK authorizer.
>
> Do you plan on including a simple implementation as part of this KIP as
> well?
>
> Gwen
>
> On Tue, Apr 10, 2018 at 10:14 PM, Magesh Nandakumar <ma...@confluent.io>
> wrote:
>
> > Hi,
> >
> > We have posted KIP-285: Connect Rest Extension Plugin to add the ability
> to
> > provide Rest Extensions to Connect Rest API.
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 285%3A+Connect+Rest+Extension+Plugin
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> >
> > Magesh
> >
>
>
>
> --
> *Gwen Shapira*
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> <http://www.confluent.io/blog>
>

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

[Gwen Shapira <gw...@confluent.io>]

Thanks Magesh! The lack of authentication and authorization in the REST API
is definitely a painpoint and something that can prevent a company from
adopting connect (or surviving an audit after they did!).

One thing that isn't clear to me from the KIP: In the past when we
contributed pluggable interfaces, we always contributed a simple
implementation that can be used for testing, experimentation and as an
example to future implementers. The JSON converter is one such example. Or
the ZK authorizer.

Do you plan on including a simple implementation as part of this KIP as
well?

Gwen

On Tue, Apr 10, 2018 at 10:14 PM, Magesh Nandakumar <ma...@confluent.io>
wrote:

> Hi,
>
> We have posted KIP-285: Connect Rest Extension Plugin to add the ability to
> provide Rest Extensions to Connect Rest API.
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 285%3A+Connect+Rest+Extension+Plugin
>
> Please take a look. Your feedback is appreciated.
>
> Thanks,
>
> Magesh
>



-- 
*Gwen Shapira*
Product Manager | Confluent
650.450.2760 | @gwenshap
Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
<http://www.confluent.io/blog>