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