Hi Randall, Thanks a lot for your feedback.
I will update the KIP to reflect your comments in (1), (2), (7) and (8). 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". 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. 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 > > >
