Yup, thanks for the changes. The 'health' package in particular feels like a nice fit given the way we expect it to be used.
-Ewen On Wed, May 16, 2018 at 7:02 PM Randall Hauch <rha...@gmail.com> wrote: > 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 > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> >
