Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Hi Jason, Thanks a lot for your feedback. The health interface does provide the same information that's already exposed in the Connect Rest endpoints. Where it would be useful is if we need to infer certain kind of health status based on the connect and task status. One good example would be

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Hi Magesh, Thanks for the KIP. It's definitely useful to have a pluggable auth layer, as we have for the kafka brokers. I was a bit unclear why we needed to expose all this health information in the context though. Since it is the bigger part of the API, I was hoping to see a little more

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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 wrote: > Looks good to me. Thanks for quickly making the changes! Great work! > > Best regards, > >

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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 wrote: > > Randall, > > I have adjusted the package names per Ewen's suggestions and also made some > minor edits per your

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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 wrote: > A few very minor

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Ewen, Thanks for your comments. I have made the changes to the package names and also moved the nested class up in the package. Public API would include *org.apache.kafka.connect.rest* -ConnectClusterState -ConnectRestExtension -ConnectRestExtensionContext *org.apache.kafka.connect.health*

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Hey, Sorry for the late follow up. I just had a couple of minor questions about details: * Some of the public API being added is under a runtime package. But that would be new for public API -- currently only things under the runtime package use that package name. I think changing the package

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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 wrote: > Hi All, > > I have updated the KIP to reflect changes based on the PR >

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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 wrote: > Thanks Randall for your thoughts. I have created a replica of the

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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,

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Randall, I'm trying to put together a PR for this KIP with the understanding that that all public interfaces should live in the api module and in the process I noticed the following 1. WorkerConfig is available in runtime. If we are exposing it to the Extension, do we just move it to the

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

On Thu, Apr 12, 2018 at 10:59 AM, Magesh Nandakumar 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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

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.

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Sounds Good. I will update the KIP to include an extension that authenticates Basic Auth Credentials against a properties file. Let me know if that works. Thanks Magesh On Wed, Apr 11, 2018 at 9:51 AM, Gwen Shapira wrote: > Not a blocker, but it will continue a good

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Not a blocker, but it will continue a good tradition to add something simple that works. I'm thinking plain-text auth and maybe ZK storage (like the Kafka authorizer). It doesn't have to provide real security in any sense, but it will allow people to test and experiment. Otherwise, if I write an

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Hi Gwen, Thanks for your feedback. As part of this KIP, I was planning on adding a skeleton implementation in the examples but that can be used only as a reference. At this moment, there is no plan to add a concrete implementation of the plugin. Let me know your thoughts :). Thanks, Magesh On

Re: [DISCUSS] KIP-285: Connect Rest Extension Plugin

Thanks Magesh! The lack of authentication and authorization in the REST API is definitely a painpoint and something that can prevent a company from adopting connect (or surviving an audit after they did!). One thing that isn't clear to me from the KIP: In the past when we contributed pluggable

[DISCUSS] KIP-285: Connect Rest Extension Plugin

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

[DISCUSS] KIP-285: Connect Rest Extension Plugin

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