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, 16 May 2018 23:49:18 GMT
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 <ewen@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 <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
> > >> > > >> >> > > >
> > >> > > >> >> > >
> > >> > > >> >> >
> > >> > > >> >>
> > >> > > >> >
> > >> > > >> >
> > >> > > >>
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

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