Looks good to me. Thanks for quickly making the changes! Great work! Best regards,
Randall > On May 16, 2018, at 8:07 PM, Magesh Nandakumar <mage...@confluent.io> wrote: > > Randall, > > I have adjusted the package names per Ewen's suggestions and also made some > minor edits per your suggestions. Since there are no major outstanding > issues, i'm moving this to vote. > > Thanks > Magesh > >> On Wed, May 16, 2018 at 4:38 PM, Randall Hauch <rha...@gmail.com> wrote: >> >> A few very minor suggestions: >> >> >> 1. There are a few formatting issues with paragraphs that use a >> monospace font. Minor, but it would be nice to fix. >> 2. Would be nice to link to the PR >> 3. Do we need the org.apache.kafka.connect.rest.extension.entities >> package? Could we just move the two classes into the parent >> org.apache.kafka.connect.rest.extension package? >> 4. This sentence "The above approach helps alleviate any issues that >> could arise if Extension accidentally reregister the" is cut off. >> 5. The "ConnectRestExtensionContext.configure(...)" method's JavaDoc >> should describe the behaviors that are mentioned in the "Rest Extension >> Integration with Connect" section; e.g., behavior when an extension >> adds a >> resource that is already registered, whether unregistering works, etc. >> Also, ideally the "close()" method would have JavaDoc that explained >> when >> it is called (e.g., no other methods will be called on the extension >> after >> this, etc.). >> 6. Packaging requirements are different for this component vs >> connectors, transformations, and converters, since this now mandates the >> Service Loader manifest file. This should be called out more explicitly. >> 7. It'd be nice if the example included how extension-specific config >> properties are to be defined in the worker configuration file. >> >> As I said, these are all minor suggestions that only affect the KIP >> document. Once these are fixed, I think this is ready to move to voting. >> >> Best regards, >> >> Randall >> >> On Tue, May 15, 2018 at 11:30 AM, Magesh Nandakumar <mage...@confluent.io> >> wrote: >> >>> 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 >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>> >>
