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/ > Configurable.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 > >> > > > > >> > > > >> > > >> > > > > >
