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