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 <e...@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 <mage...@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 <mage...@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 <rha...@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 < > > mage...@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 <rha...@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 <rha...@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 < > > >> > > mage...@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 < > > >> > > mage...@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 < > > rha...@gmail.com > > >> > > > >> > > >> wrote: > > >> > > >> > > > >> > > >> >> On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar < > > >> > > >> mage...@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 < > > >> > rha...@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 < > > >> > > >> >> > mage...@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 > > >> > > >> >> > > > > > >> > > >> >> > > > > >> > > >> >> > > > >> > > >> >> > > >> > > >> > > > >> > > >> > > > >> > > >> > > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
