Randall, I'm trying to put together a PR for this KIP with the understanding that that all public interfaces should live in the api module and in the process I noticed the following
1. WorkerConfig is available in runtime. If we are exposing it to the Extension, do we just move it to the API module? 2. All of the rest entities today live in runtime. We are exposing org.apache.kafka.connect.runtime.rest.entities.ConnectorStateInfo via our plugin interface. So , was wondering if these entities can move to API since they are already in a way public. If we don't want to do that all the new interfaces would need to live in the runtime. This might affect some minor details in the KIP. Let me know your thoughts. Thanks Magesh On Thu, Apr 19, 2018 at 10:17 PM, 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 >>> > > > >>> > > >>> > >>> >> >> >
