kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Magesh Nandakumar <mage...@confluent.io>
Subject Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin
Date Wed, 25 Apr 2018 20:03:09 GMT
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
> >> >> > > >
> >> >> > >
> >> >> >
> >> >>
> >> >
> >> >
> >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message