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 <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 > >> > > >> >> > > > > >> > > >> >> > > > >> > > >> >> > > >> > > >> >> > >> > > >> > > >> > > >> > > >> > > >> > >> > > > > >> > > > > >> > > > >> > > >> > > > > >
