kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Randall Hauch <rha...@gmail.com>
Subject Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin
Date Fri, 27 Apr 2018 19:03:16 GMT
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
> > >> >> > > >
> > >> >> > >
> > >> >> >
> > >> >>
> > >> >
> > >> >
> > >>
> > >
> > >
> >
>

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