Re: [DISCUSS] KIP-507: Securing Internal Connect REST Endpoints

[Magesh Nandakumar]

Hi Chris,

Thanks a lot for the KIP. This will certainly be a useful feature. I would
have preferred to use the topic approach as well but I also understand your
point of view about the operational complexity for upgrades. If not with
this KIP, I would certainly want to go that route at some point in the
future.

As far as using the rebalance protocol goes, it would be great if you could
elaborate on what exactly would be the rebalance impact when a key expires.
I see that you have called out saying that there should be no significant
impact but it will be great to explicitly state as what is to be expected.
I would prefer to not have any sorts of rebalancing when this happens since
the connector and task assignments should not change with this event. It
will be useful to explain this a bit more.

Thanks,
Magesh

On Wed, Aug 21, 2019 at 4:40 PM Chris Egerton  wrote:

> Hi all,
>
> I've made some tweaks to the KIP that I believe are improvements. More
> detail can be found on the KIP page itself, but as a brief summary, the
> three changes are:
>
> - The removal of the internal.request.verification property in favor of
> modifying the default value for the connect.protocol property from
> "compatible" to "sessioned"
> - The renaming of some configurations to use better terminology (mostly
> just "request" instead of "key" where appropriate)
> - The addition of two new configurations that dictate how session keys are
> to be generated
>
> Thanks Ryanne for the feedback so far, hope to hear from some more of you
> soon :)
>
> Cheers,
>
> Chris
>
> On Mon, Aug 19, 2019 at 12:02 PM Chris Egerton 
> wrote:
>
> > Hi Ryanne,
> >
> > The reasoning for this is provided in the KIP: "There would be no clear
> > way to achieve consensus amongst the workers in a cluster on whether to
> > switch to this new behavior." To elaborate on this--it would be bad if a
> > follower worker began writing task configurations to the topic while the
> > leader continued to only listen on the internal REST endpoint. It's
> > necessary to ensure that every worker in a cluster supports this new
> > behavior before switching it on.
> >
> > To be fair, it is technically possible to achieve consensus without using
> > the group coordination protocol by adding new configurations and using a
> > multi-phase rolling upgrade, but the operational complexity of this
> > approach would significantly complicate things for the default case of
> just
> > wanting to bump your Connect cluster to the next version without having
> to
> > alter your existing worker config files and with only a single restart
> per
> > worker. The proposed approach provides a much simpler user experience for
> > the most common use case and doesn't impose much additional complexity
> for
> > other use cases.
> >
> > I've updated the KIP to expand on points from your last emails; let me
> > know what you think.
> >
> > Cheers,
> >
> > Chris
> >
> > On Thu, Aug 15, 2019 at 4:53 PM Ryanne Dolan 
> > wrote:
> >
> >> Thanks Chris, that makes sense.
> >>
> >> I know you have already considered this, but I'm not convinced we should
> >> rule out using Kafka topics for this purpose. That would enable the same
> >> level of security without introducing any new authentication or
> >> authorization mechanisms (your keys). And as you say, you'd need to lock
> >> down Connect's topics and groups anyway.
> >>
> >> Can you explain what you mean when you say using Kafka topics would
> >> require
> >> "reworking the group coordination protocol"? I don't see how these are
> >> related. Why would it matter if the workers sent Kafka messages vs POST
> >> requests among themselves?
> >>
> >> Ryanne
> >>
> >> On Thu, Aug 15, 2019 at 3:57 PM Chris Egerton 
> >> wrote:
> >>
> >> > Hi Ryanne,
> >> >
> >> > Yes, if the Connect group is left unsecured then that is a potential
> >> > vulnerability. However, in a truly secure Connect cluster, the group
> >> would
> >> > need to be secured anyways to prevent attackers from joining the group
> >> with
> >> > the intent to either snoop on connector/task configurations or bring
> the
> >> > cluster to a halt by spamming the group with membership requests and
> >> then
> >> > not running the assigned connectors/tasks. Additionally, for a Connect
> >> > cluster to be secure, access to internal topics (for configs, offsets,
> >> and
> >> > statuses) would also need to be restricted so that attackers could
> not,
> >> > e.g., write arbitrary connector/task configurations to the configs
> >> topic.
> >> > This is all currently possible in Kafka with the use of ACLs.
> >> >
> >> > I think the bottom line here is that there's a number of steps that
> >> need to
> >> > be taken to effectively lock down a Connect cluster; the point of this
> >> KIP
> >> > is to close a loophole that exists even after all of those steps are
> >> taken,
> >> > not to completely secure this one vulnerability even when no other
> >> security
> >> > measures are taken.
> >> >
> >> > 

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

While implementing the KIP it became evident that for the validate call
from the Connect Rest interface it was required to pass each config
individually to the policy implementation to be able to determine which
overrides are allowed. This happened since the interface relied on using
Exception(PolicyViolationException) as the mechanism to indicate an error
in the configuration. This defeats the purpose of the interface which
allows all overridden configurations to be passed together. To overcome
this limitation, I have updated the KIP (specifically the interface) to
return a list of ConfigValue (org.apache.kafka.common.config.ConfigValue )
instead of throwing an exception. This pattern is already followed in the
validate() of the Connector class. This change doesn't alter the semantics
or behavior of the out-of-the-box policies or how policies are configured
or enforced. Let me know if you have any questions or comments on the
change.

Apart from the above interface change, I have also made another minor
change which was missed out in the list of Configs allowed by the
`Principal` policy. I have included `security.protocol` and
`sasl.mechanism` which were missed out.

On Fri, May 10, 2019 at 9:23 AM Magesh Nandakumar 
wrote:

> Thanks a lot, Colin.  This KIP has now passed voting with 3 binding votes
> ( Randall, Rajini & Colin) and 1 non-binding vote (Chris).
> Thanks a lot, everyone for the feedback & discussion on this KIP.
>
> On Fri, May 10, 2019 at 9:12 AM Colin McCabe  wrote:
>
>> +1 (binding).  Thanks, Magesh.
>>
>> cheers,
>> Colin
>>
>> On Thu, May 9, 2019, at 18:31, Randall Hauch wrote:
>> > I'm still +1 and like the simplification.
>> >
>> > Randall
>> >
>> > On Thu, May 9, 2019 at 5:54 PM Magesh Nandakumar 
>> > wrote:
>> >
>> > > I have updated the KIP to remove the `Ignore` policy and also the
>> > > useOverrides()
>> > > method in the interface.
>> > > Thanks a lot for your thoughts, Colin. I believe this certainly
>> simplifies
>> > > the KIP.
>> > >
>> > > On Thu, May 9, 2019 at 3:44 PM Magesh Nandakumar <
>> mage...@confluent.io>
>> > > wrote:
>> > >
>> > > > Unless anyone has objections, I'm going to update the KIP to remove
>> the
>> > > > `Ignore` policy and make `None` as the default. I will also remove
>> the `
>> > > > default boolean useOverrides()` in the interface which was
>> introduced for
>> > > > the purpose of backward compatibility.
>> > > >
>> > > > On Thu, May 9, 2019 at 3:27 PM Randall Hauch 
>> wrote:
>> > > >
>> > > >> I have also seen users include in connector configs the
>> `producer.*` and
>> > > >> `consumer.*` properties that should go into the worker configs. But
>> > > those
>> > > >> don't match, and the likelihood that someone is already using
>> > > >> `producer.override.*` or `consumer.override.*` properties in their
>> > > >> connector configs does seem pretty tiny.
>> > > >>
>> > > >> I'd be fine with removing the `Ignore` for backward compatibility.
>> Still
>> > > >> +1
>> > > >> either way.
>> > > >>
>> > > >> On Thu, May 9, 2019 at 5:23 PM Magesh Nandakumar <
>> mage...@confluent.io>
>> > > >> wrote:
>> > > >>
>> > > >> > To add more details regarding the backward compatibility; I have
>> > > >> generally
>> > > >> > seen users trying to set "producer.request.timeout.ms
>> > > >> > <http://producer.override.request.timeout.ms/>" in their
>> connector
>> > > >> config
>> > > >> > under the assumption that it will get used and would never come
>> back
>> > > to
>> > > >> > remove it. The initial intent of the KIP was to use the same
>> prefix
>> > > but
>> > > >> > since that potentially collided with MM2 configs, we agreed to
>> use a
>> > > >> > different prefix "producer.override". With this context, I think
>> the
>> > > >> > likelihood of someone using this is very small and should
>> generally
>> > > not
>> > > >> be
>> > > >> > a problem.
>> > > >> >
>> > > >> > On Thu, May 9, 2019 at 3:15 PM Magesh Nandak

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Thanks a lot, Colin.  This KIP has now passed voting with 3 binding votes (
Randall, Rajini & Colin) and 1 non-binding vote (Chris).
Thanks a lot, everyone for the feedback & discussion on this KIP.

On Fri, May 10, 2019 at 9:12 AM Colin McCabe  wrote:

> +1 (binding).  Thanks, Magesh.
>
> cheers,
> Colin
>
> On Thu, May 9, 2019, at 18:31, Randall Hauch wrote:
> > I'm still +1 and like the simplification.
> >
> > Randall
> >
> > On Thu, May 9, 2019 at 5:54 PM Magesh Nandakumar 
> > wrote:
> >
> > > I have updated the KIP to remove the `Ignore` policy and also the
> > > useOverrides()
> > > method in the interface.
> > > Thanks a lot for your thoughts, Colin. I believe this certainly
> simplifies
> > > the KIP.
> > >
> > > On Thu, May 9, 2019 at 3:44 PM Magesh Nandakumar  >
> > > wrote:
> > >
> > > > Unless anyone has objections, I'm going to update the KIP to remove
> the
> > > > `Ignore` policy and make `None` as the default. I will also remove
> the `
> > > > default boolean useOverrides()` in the interface which was
> introduced for
> > > > the purpose of backward compatibility.
> > > >
> > > > On Thu, May 9, 2019 at 3:27 PM Randall Hauch 
> wrote:
> > > >
> > > >> I have also seen users include in connector configs the
> `producer.*` and
> > > >> `consumer.*` properties that should go into the worker configs. But
> > > those
> > > >> don't match, and the likelihood that someone is already using
> > > >> `producer.override.*` or `consumer.override.*` properties in their
> > > >> connector configs does seem pretty tiny.
> > > >>
> > > >> I'd be fine with removing the `Ignore` for backward compatibility.
> Still
> > > >> +1
> > > >> either way.
> > > >>
> > > >> On Thu, May 9, 2019 at 5:23 PM Magesh Nandakumar <
> mage...@confluent.io>
> > > >> wrote:
> > > >>
> > > >> > To add more details regarding the backward compatibility; I have
> > > >> generally
> > > >> > seen users trying to set "producer.request.timeout.ms
> > > >> > <http://producer.override.request.timeout.ms/>" in their
> connector
> > > >> config
> > > >> > under the assumption that it will get used and would never come
> back
> > > to
> > > >> > remove it. The initial intent of the KIP was to use the same
> prefix
> > > but
> > > >> > since that potentially collided with MM2 configs, we agreed to
> use a
> > > >> > different prefix "producer.override". With this context, I think
> the
> > > >> > likelihood of someone using this is very small and should
> generally
> > > not
> > > >> be
> > > >> > a problem.
> > > >> >
> > > >> > On Thu, May 9, 2019 at 3:15 PM Magesh Nandakumar <
> > > mage...@confluent.io>
> > > >> > wrote:
> > > >> >
> > > >> > > Colin,
> > > >> > >
> > > >> > > Thanks a lot for the feedback.  As you said, the possibilities
> of
> > > >> someone
> > > >> > > having "producer.override.request.timeout.ms" in their
> connector
> > > >> config
> > > >> > > in AK 2.2 or lower is very slim. But the key thing is if in
> case,
> > > >> someone
> > > >> > > has it AK2.2 doesn't do anything with it and it silently
> ignores the
> > > >> > > configuration. If others think that it's not really a problem,
> then
> > > >> I'm
> > > >> > > fine with removing the complicated compatibility issue.
> > > >> > >
> > > >> > > I have explicitly called out the behavior when the exception is
> > > >> thrown.
> > > >> > >
> > > >> > > Let me know what you think.
> > > >> > >
> > > >> > > Thanks,
> > > >> > > Magesh
> > > >> > >
> > > >> > > On Thu, May 9, 2019 at 2:45 PM Colin McCabe  >
> > > >> wrote:
> > > >> > >
> > > >> > >> Hi Magesh,
> > > >> > >>
> > > >> > >>

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

I have updated the KIP to remove the `Ignore` policy and also the
useOverrides()
method in the interface.
Thanks a lot for your thoughts, Colin. I believe this certainly simplifies
the KIP.

On Thu, May 9, 2019 at 3:44 PM Magesh Nandakumar 
wrote:

> Unless anyone has objections, I'm going to update the KIP to remove the
> `Ignore` policy and make `None` as the default. I will also remove the `
> default boolean useOverrides()` in the interface which was introduced for
> the purpose of backward compatibility.
>
> On Thu, May 9, 2019 at 3:27 PM Randall Hauch  wrote:
>
>> I have also seen users include in connector configs the `producer.*` and
>> `consumer.*` properties that should go into the worker configs. But those
>> don't match, and the likelihood that someone is already using
>> `producer.override.*` or `consumer.override.*` properties in their
>> connector configs does seem pretty tiny.
>>
>> I'd be fine with removing the `Ignore` for backward compatibility. Still
>> +1
>> either way.
>>
>> On Thu, May 9, 2019 at 5:23 PM Magesh Nandakumar 
>> wrote:
>>
>> > To add more details regarding the backward compatibility; I have
>> generally
>> > seen users trying to set "producer.request.timeout.ms
>> > <http://producer.override.request.timeout.ms/>" in their connector
>> config
>> > under the assumption that it will get used and would never come back to
>> > remove it. The initial intent of the KIP was to use the same prefix but
>> > since that potentially collided with MM2 configs, we agreed to use a
>> > different prefix "producer.override". With this context, I think the
>> > likelihood of someone using this is very small and should generally not
>> be
>> > a problem.
>> >
>> > On Thu, May 9, 2019 at 3:15 PM Magesh Nandakumar 
>> > wrote:
>> >
>> > > Colin,
>> > >
>> > > Thanks a lot for the feedback.  As you said, the possibilities of
>> someone
>> > > having "producer.override.request.timeout.ms" in their connector
>> config
>> > > in AK 2.2 or lower is very slim. But the key thing is if in case,
>> someone
>> > > has it AK2.2 doesn't do anything with it and it silently ignores the
>> > > configuration. If others think that it's not really a problem, then
>> I'm
>> > > fine with removing the complicated compatibility issue.
>> > >
>> > > I have explicitly called out the behavior when the exception is
>> thrown.
>> > >
>> > > Let me know what you think.
>> > >
>> > > Thanks,
>> > > Magesh
>> > >
>> > > On Thu, May 9, 2019 at 2:45 PM Colin McCabe 
>> wrote:
>> > >
>> > >> Hi Magesh,
>> > >>
>> > >> Thanks for the KIP.  It looks good overall.
>> > >>
>> > >> >default boolean useOverrides() {
>> > >> >return true;
>> > >> >}
>> > >>
>> > >> Is this method really needed?  As I understand, nobody should have
>> any
>> > >> connector client config overrides set right now, since they don't do
>> > >> anything right now.
>> > >>
>> > >> For example, you wouldn't expect a Kafka 2.2 installation to have "
>> > >> producer.override.request.timeout.ms" set, since that doesn't do
>> > >> anything in Kafka 2.2.  So is the option to ignore it in Kafka 2.3
>> > really
>> > >> necessary?
>> > >>
>> > >> Can you add some details about what happens if a
>> > >> PolicyValidationException is thrown?  I'm assuming that we fail to
>> > create
>> > >> the new Connector, I'm not sure if that's completely spelled out
>> > (unless I
>> > >> missed it).
>> > >>
>> > >> best,
>> > >> Colin
>> > >>
>> > >>
>> > >> On Thu, May 9, 2019, at 08:05, Rajini Sivaram wrote:
>> > >> > Hi Magesh,
>> > >> >
>> > >> > Thanks for the KIP, +1 (binding)
>> > >> >
>> > >> > Regards,
>> > >> >
>> > >> > Rajini
>> > >> >
>> > >> >
>> > >> > On Thu, May 9, 2019 at 3:55 PM Randall Hauch 
>> > wrote:
>> > >> >
>> > >> > > Nice work, Magesh.
>

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Unless anyone has objections, I'm going to update the KIP to remove the
`Ignore` policy and make `None` as the default. I will also remove the `
default boolean useOverrides()` in the interface which was introduced for
the purpose of backward compatibility.

On Thu, May 9, 2019 at 3:27 PM Randall Hauch  wrote:

> I have also seen users include in connector configs the `producer.*` and
> `consumer.*` properties that should go into the worker configs. But those
> don't match, and the likelihood that someone is already using
> `producer.override.*` or `consumer.override.*` properties in their
> connector configs does seem pretty tiny.
>
> I'd be fine with removing the `Ignore` for backward compatibility. Still +1
> either way.
>
> On Thu, May 9, 2019 at 5:23 PM Magesh Nandakumar 
> wrote:
>
> > To add more details regarding the backward compatibility; I have
> generally
> > seen users trying to set "producer.request.timeout.ms
> > <http://producer.override.request.timeout.ms/>" in their connector
> config
> > under the assumption that it will get used and would never come back to
> > remove it. The initial intent of the KIP was to use the same prefix but
> > since that potentially collided with MM2 configs, we agreed to use a
> > different prefix "producer.override". With this context, I think the
> > likelihood of someone using this is very small and should generally not
> be
> > a problem.
> >
> > On Thu, May 9, 2019 at 3:15 PM Magesh Nandakumar 
> > wrote:
> >
> > > Colin,
> > >
> > > Thanks a lot for the feedback.  As you said, the possibilities of
> someone
> > > having "producer.override.request.timeout.ms" in their connector
> config
> > > in AK 2.2 or lower is very slim. But the key thing is if in case,
> someone
> > > has it AK2.2 doesn't do anything with it and it silently ignores the
> > > configuration. If others think that it's not really a problem, then I'm
> > > fine with removing the complicated compatibility issue.
> > >
> > > I have explicitly called out the behavior when the exception is thrown.
> > >
> > > Let me know what you think.
> > >
> > > Thanks,
> > > Magesh
> > >
> > > On Thu, May 9, 2019 at 2:45 PM Colin McCabe 
> wrote:
> > >
> > >> Hi Magesh,
> > >>
> > >> Thanks for the KIP.  It looks good overall.
> > >>
> > >> >default boolean useOverrides() {
> > >> >return true;
> > >> >}
> > >>
> > >> Is this method really needed?  As I understand, nobody should have any
> > >> connector client config overrides set right now, since they don't do
> > >> anything right now.
> > >>
> > >> For example, you wouldn't expect a Kafka 2.2 installation to have "
> > >> producer.override.request.timeout.ms" set, since that doesn't do
> > >> anything in Kafka 2.2.  So is the option to ignore it in Kafka 2.3
> > really
> > >> necessary?
> > >>
> > >> Can you add some details about what happens if a
> > >> PolicyValidationException is thrown?  I'm assuming that we fail to
> > create
> > >> the new Connector, I'm not sure if that's completely spelled out
> > (unless I
> > >> missed it).
> > >>
> > >> best,
> > >> Colin
> > >>
> > >>
> > >> On Thu, May 9, 2019, at 08:05, Rajini Sivaram wrote:
> > >> > Hi Magesh,
> > >> >
> > >> > Thanks for the KIP, +1 (binding)
> > >> >
> > >> > Regards,
> > >> >
> > >> > Rajini
> > >> >
> > >> >
> > >> > On Thu, May 9, 2019 at 3:55 PM Randall Hauch 
> > wrote:
> > >> >
> > >> > > Nice work, Magesh.
> > >> > >
> > >> > > +1 (binding)
> > >> > >
> > >> > > Randall
> > >> > >
> > >> > > On Wed, May 8, 2019 at 7:22 PM Magesh Nandakumar <
> > >> mage...@confluent.io>
> > >> > > wrote:
> > >> > >
> > >> > > > Thanks a lot Chris. So far, the KIP has one non-binding vote and
> > I'm
> > >> > > still
> > >> > > > looking forward to the KIP to be voted by Friday's deadline.
> > >> > > >
> > >> > > > On Tue, May 7, 2019 at 10:00 AM Chris Egerton <
> > chr...@confluent.io>
> > >> > > wrote:
> > >> > > >
> > >> > > > > Hi Magesh,
> > >> > > > >
> > >> > > > > This looks great! Very excited to see these changes finally
> > >> coming to
> > >> > > > > Connect.
> > >> > > > > +1 (non-binding)
> > >> > > > >
> > >> > > > > Cheers,
> > >> > > > >
> > >> > > > > Chris
> > >> > > > >
> > >> > > > > On Tue, May 7, 2019 at 9:51 AM Magesh Nandakumar <
> > >> mage...@confluent.io
> > >> > > >
> > >> > > > > wrote:
> > >> > > > >
> > >> > > > > > Hi All,
> > >> > > > > >
> > >> > > > > > I would like to start a vote on
> > >> > > > > >
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
> > >> > > > > >
> > >> > > > > > The discussion thread can be found here
> > >> > > > > > <
> > >> https://www.mail-archive.com/dev@kafka.apache.org/msg97124.html>.
> > >> > > > > >
> > >> > > > > > Thanks,
> > >> > > > > > Magesh
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

To add more details regarding the backward compatibility; I have generally
seen users trying to set "producer.request.timeout.ms
<http://producer.override.request.timeout.ms/>" in their connector config
under the assumption that it will get used and would never come back to
remove it. The initial intent of the KIP was to use the same prefix but
since that potentially collided with MM2 configs, we agreed to use a
different prefix "producer.override". With this context, I think the
likelihood of someone using this is very small and should generally not be
a problem.

On Thu, May 9, 2019 at 3:15 PM Magesh Nandakumar 
wrote:

> Colin,
>
> Thanks a lot for the feedback.  As you said, the possibilities of someone
> having "producer.override.request.timeout.ms" in their connector config
> in AK 2.2 or lower is very slim. But the key thing is if in case, someone
> has it AK2.2 doesn't do anything with it and it silently ignores the
> configuration. If others think that it's not really a problem, then I'm
> fine with removing the complicated compatibility issue.
>
> I have explicitly called out the behavior when the exception is thrown.
>
> Let me know what you think.
>
> Thanks,
> Magesh
>
> On Thu, May 9, 2019 at 2:45 PM Colin McCabe  wrote:
>
>> Hi Magesh,
>>
>> Thanks for the KIP.  It looks good overall.
>>
>> >default boolean useOverrides() {
>> >return true;
>> >}
>>
>> Is this method really needed?  As I understand, nobody should have any
>> connector client config overrides set right now, since they don't do
>> anything right now.
>>
>> For example, you wouldn't expect a Kafka 2.2 installation to have "
>> producer.override.request.timeout.ms" set, since that doesn't do
>> anything in Kafka 2.2.  So is the option to ignore it in Kafka 2.3 really
>> necessary?
>>
>> Can you add some details about what happens if a
>> PolicyValidationException is thrown?  I'm assuming that we fail to create
>> the new Connector, I'm not sure if that's completely spelled out (unless I
>> missed it).
>>
>> best,
>> Colin
>>
>>
>> On Thu, May 9, 2019, at 08:05, Rajini Sivaram wrote:
>> > Hi Magesh,
>> >
>> > Thanks for the KIP, +1 (binding)
>> >
>> > Regards,
>> >
>> > Rajini
>> >
>> >
>> > On Thu, May 9, 2019 at 3:55 PM Randall Hauch  wrote:
>> >
>> > > Nice work, Magesh.
>> > >
>> > > +1 (binding)
>> > >
>> > > Randall
>> > >
>> > > On Wed, May 8, 2019 at 7:22 PM Magesh Nandakumar <
>> mage...@confluent.io>
>> > > wrote:
>> > >
>> > > > Thanks a lot Chris. So far, the KIP has one non-binding vote and I'm
>> > > still
>> > > > looking forward to the KIP to be voted by Friday's deadline.
>> > > >
>> > > > On Tue, May 7, 2019 at 10:00 AM Chris Egerton 
>> > > wrote:
>> > > >
>> > > > > Hi Magesh,
>> > > > >
>> > > > > This looks great! Very excited to see these changes finally
>> coming to
>> > > > > Connect.
>> > > > > +1 (non-binding)
>> > > > >
>> > > > > Cheers,
>> > > > >
>> > > > > Chris
>> > > > >
>> > > > > On Tue, May 7, 2019 at 9:51 AM Magesh Nandakumar <
>> mage...@confluent.io
>> > > >
>> > > > > wrote:
>> > > > >
>> > > > > > Hi All,
>> > > > > >
>> > > > > > I would like to start a vote on
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
>> > > > > >
>> > > > > > The discussion thread can be found here
>> > > > > > <
>> https://www.mail-archive.com/dev@kafka.apache.org/msg97124.html>.
>> > > > > >
>> > > > > > Thanks,
>> > > > > > Magesh
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Colin,

Thanks a lot for the feedback.  As you said, the possibilities of someone
having "producer.override.request.timeout.ms" in their connector config in
AK 2.2 or lower is very slim. But the key thing is if in case, someone has
it AK2.2 doesn't do anything with it and it silently ignores the
configuration. If others think that it's not really a problem, then I'm
fine with removing the complicated compatibility issue.

I have explicitly called out the behavior when the exception is thrown.

Let me know what you think.

Thanks,
Magesh

On Thu, May 9, 2019 at 2:45 PM Colin McCabe  wrote:

> Hi Magesh,
>
> Thanks for the KIP.  It looks good overall.
>
> >default boolean useOverrides() {
> >return true;
> >}
>
> Is this method really needed?  As I understand, nobody should have any
> connector client config overrides set right now, since they don't do
> anything right now.
>
> For example, you wouldn't expect a Kafka 2.2 installation to have "
> producer.override.request.timeout.ms" set, since that doesn't do anything
> in Kafka 2.2.  So is the option to ignore it in Kafka 2.3 really necessary?
>
> Can you add some details about what happens if a PolicyValidationException
> is thrown?  I'm assuming that we fail to create the new Connector, I'm not
> sure if that's completely spelled out (unless I missed it).
>
> best,
> Colin
>
>
> On Thu, May 9, 2019, at 08:05, Rajini Sivaram wrote:
> > Hi Magesh,
> >
> > Thanks for the KIP, +1 (binding)
> >
> > Regards,
> >
> > Rajini
> >
> >
> > On Thu, May 9, 2019 at 3:55 PM Randall Hauch  wrote:
> >
> > > Nice work, Magesh.
> > >
> > > +1 (binding)
> > >
> > > Randall
> > >
> > > On Wed, May 8, 2019 at 7:22 PM Magesh Nandakumar  >
> > > wrote:
> > >
> > > > Thanks a lot Chris. So far, the KIP has one non-binding vote and I'm
> > > still
> > > > looking forward to the KIP to be voted by Friday's deadline.
> > > >
> > > > On Tue, May 7, 2019 at 10:00 AM Chris Egerton 
> > > wrote:
> > > >
> > > > > Hi Magesh,
> > > > >
> > > > > This looks great! Very excited to see these changes finally coming
> to
> > > > > Connect.
> > > > > +1 (non-binding)
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Chris
> > > > >
> > > > > On Tue, May 7, 2019 at 9:51 AM Magesh Nandakumar <
> mage...@confluent.io
> > > >
> > > > > wrote:
> > > > >
> > > > > > Hi All,
> > > > > >
> > > > > > I would like to start a vote on
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
> > > > > >
> > > > > > The discussion thread can be found here
> > > > > > <https://www.mail-archive.com/dev@kafka.apache.org/msg97124.html
> >.
> > > > > >
> > > > > > Thanks,
> > > > > > Magesh
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Thanks a lot Chris. So far, the KIP has one non-binding vote and I'm still
looking forward to the KIP to be voted by Friday's deadline.

On Tue, May 7, 2019 at 10:00 AM Chris Egerton  wrote:

> Hi Magesh,
>
> This looks great! Very excited to see these changes finally coming to
> Connect.
> +1 (non-binding)
>
> Cheers,
>
> Chris
>
> On Tue, May 7, 2019 at 9:51 AM Magesh Nandakumar 
> wrote:
>
> > Hi All,
> >
> > I would like to start a vote on
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
> >
> > The discussion thread can be found here
> > <https://www.mail-archive.com/dev@kafka.apache.org/msg97124.html>.
> >
> > Thanks,
> > Magesh
> >
>

[VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi All,

I would like to start a vote on
https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy

The discussion thread can be found here
.

Thanks,
Magesh

[VOTE] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi All,

I would like to start a vote on
https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy

The discussion thread can be found here
<https://www.mail-archive.com/dev@kafka.apache.org/msg97124.html>.

Thanks,
Magesh

On Tue, May 7, 2019 at 9:35 AM Magesh Nandakumar 
wrote:

> I have addressed all the outstanding discussion points and I believe we
> have consensus on the design. Thanks, everyone for the feedback. I will
> start a VOTE thread on this KIP.
>
> On Mon, May 6, 2019 at 10:13 PM Magesh Nandakumar 
> wrote:
>
>> Randall,
>>
>> Thanks a lot for the suggestions. I have incorporated the comments in the
>> KIP.
>>
>> Thanks,
>> Magesh
>>
>> On Mon, May 6, 2019 at 6:52 PM Randall Hauch  wrote:
>>
>>> Thanks, Magesh. I do have a few pretty minor suggestions.
>>>
>>> 1) Define a bit more clearly in the "Proposed Changes" whether the
>>> configs
>>> passed to the validate method via the ConnectorClientConfigRequest object
>>> have or do not have the prefixes.
>>> 2) Specify more clearly in (or around) the table which is the default
>>> policy. Currently the Ignore policy "Behavior" just mentions that it's
>>> the
>>> current behavior, but I think it would help that it is described as the
>>> default for the property.
>>>
>>> Otherwise, this looks good to me.
>>>
>>> Best regards,
>>>
>>> Randall
>>>
>>> On Mon, May 6, 2019 at 8:12 PM Magesh Nandakumar 
>>> wrote:
>>>
>>> > Konstantine,
>>> >
>>> > Thanks a lot for your feedback on the KIP. I have incorporated the
>>> feedback
>>> > using generics for Class. I have also updated the KIP to handle the
>>> default
>>> > value per Randall's suggestion. Let me know if you have any questions.
>>> >
>>> > Thanks,
>>> > Magesh
>>> >
>>> >
>>> > On Mon, May 6, 2019 at 1:58 PM Konstantine Karantasis <
>>> > konstant...@confluent.io> wrote:
>>> >
>>> > > Thanks for the KIP Magesh, it's quite useful towards the goals for
>>> more
>>> > > general multi-tenancy in Connect.
>>> > >
>>> > > Couple of comments from me too:
>>> > >
>>> > > I think the fact that the default policy is 'null' (no
>>> implementation)
>>> > > should be mentioned on the table next to the available
>>> implementations.
>>> > > Currently the KIP says: 'In addition to the default implementation,
>>> ..."
>>> > > but this is not very accurate because there is no concrete default
>>> > > implementation. Just special handling of 'null' in
>>> > > 'connector.client.config.policy'
>>> > >
>>> > > Regarding passing the overrides to the connector 'configure' method,
>>> I
>>> > feel
>>> > > it wouldn't hurt to pass them, but I also agree that leaving this
>>> out at
>>> > > the moment is the safest option.
>>> > >
>>> > > Since the interfaces and classes are listed in the KIP, I'd like to
>>> note
>>> > > that Class is used as a raw type in field and return value
>>> declarations.
>>> > We
>>> > > should use the generic type instead.
>>> > >
>>> > > Thanks for this improvement proposal!
>>> > > Konstantine
>>> > >
>>> > > On Mon, May 6, 2019 at 11:11 AM Magesh Nandakumar <
>>> mage...@confluent.io>
>>> > > wrote:
>>> > >
>>> > > > Randall,
>>> > > >
>>> > > > I was wondering if you had any thoughts on the above alternatives
>>> to
>>> > deal
>>> > > > with a default policy.  If it's possible, I would like to finalize
>>> the
>>> > > > discussions and start a vote.
>>> > > > Let me know your thoughts.
>>> > > >
>>> > > > Thanks,
>>> > > > Magesh
>>> > > >
>>> > > > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar <
>>> > mage...@confluent.io>
>>> > > > wrote:
>>> > > >
>>> > > > > Randall,
>>> > > > >
>>> > > > > The approach to return the 

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

I have addressed all the outstanding discussion points and I believe we
have consensus on the design. Thanks, everyone for the feedback. I will
start a VOTE thread on this KIP.

On Mon, May 6, 2019 at 10:13 PM Magesh Nandakumar 
wrote:

> Randall,
>
> Thanks a lot for the suggestions. I have incorporated the comments in the
> KIP.
>
> Thanks,
> Magesh
>
> On Mon, May 6, 2019 at 6:52 PM Randall Hauch  wrote:
>
>> Thanks, Magesh. I do have a few pretty minor suggestions.
>>
>> 1) Define a bit more clearly in the "Proposed Changes" whether the configs
>> passed to the validate method via the ConnectorClientConfigRequest object
>> have or do not have the prefixes.
>> 2) Specify more clearly in (or around) the table which is the default
>> policy. Currently the Ignore policy "Behavior" just mentions that it's the
>> current behavior, but I think it would help that it is described as the
>> default for the property.
>>
>> Otherwise, this looks good to me.
>>
>> Best regards,
>>
>> Randall
>>
>> On Mon, May 6, 2019 at 8:12 PM Magesh Nandakumar 
>> wrote:
>>
>> > Konstantine,
>> >
>> > Thanks a lot for your feedback on the KIP. I have incorporated the
>> feedback
>> > using generics for Class. I have also updated the KIP to handle the
>> default
>> > value per Randall's suggestion. Let me know if you have any questions.
>> >
>> > Thanks,
>> > Magesh
>> >
>> >
>> > On Mon, May 6, 2019 at 1:58 PM Konstantine Karantasis <
>> > konstant...@confluent.io> wrote:
>> >
>> > > Thanks for the KIP Magesh, it's quite useful towards the goals for
>> more
>> > > general multi-tenancy in Connect.
>> > >
>> > > Couple of comments from me too:
>> > >
>> > > I think the fact that the default policy is 'null' (no implementation)
>> > > should be mentioned on the table next to the available
>> implementations.
>> > > Currently the KIP says: 'In addition to the default implementation,
>> ..."
>> > > but this is not very accurate because there is no concrete default
>> > > implementation. Just special handling of 'null' in
>> > > 'connector.client.config.policy'
>> > >
>> > > Regarding passing the overrides to the connector 'configure' method, I
>> > feel
>> > > it wouldn't hurt to pass them, but I also agree that leaving this out
>> at
>> > > the moment is the safest option.
>> > >
>> > > Since the interfaces and classes are listed in the KIP, I'd like to
>> note
>> > > that Class is used as a raw type in field and return value
>> declarations.
>> > We
>> > > should use the generic type instead.
>> > >
>> > > Thanks for this improvement proposal!
>> > > Konstantine
>> > >
>> > > On Mon, May 6, 2019 at 11:11 AM Magesh Nandakumar <
>> mage...@confluent.io>
>> > > wrote:
>> > >
>> > > > Randall,
>> > > >
>> > > > I was wondering if you had any thoughts on the above alternatives to
>> > deal
>> > > > with a default policy.  If it's possible, I would like to finalize
>> the
>> > > > discussions and start a vote.
>> > > > Let me know your thoughts.
>> > > >
>> > > > Thanks,
>> > > > Magesh
>> > > >
>> > > > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar <
>> > mage...@confluent.io>
>> > > > wrote:
>> > > >
>> > > > > Randall,
>> > > > >
>> > > > > The approach to return the to override configs could possibly
>> make it
>> > > > > cumbersome to implement a custom policy. This is a new
>> configuration
>> > > and
>> > > > if
>> > > > > you don't explicitly set it the existing behavior remains as-is.
>> Like
>> > > > > Chris, I also preferred this approach for the sake of
>> simplicity.  If
>> > > not
>> > > > > for the default `null` I would prefer to fall back to using
>> `Ignore`
>> > > > which
>> > > > > is a misnomer to the interface spec but still gets the job done
>> via
>> > > > > instanceOf checks. The other options I could think of are as
>> below:-
>> > &

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Randall,

Thanks a lot for the suggestions. I have incorporated the comments in the
KIP.

Thanks,
Magesh

On Mon, May 6, 2019 at 6:52 PM Randall Hauch  wrote:

> Thanks, Magesh. I do have a few pretty minor suggestions.
>
> 1) Define a bit more clearly in the "Proposed Changes" whether the configs
> passed to the validate method via the ConnectorClientConfigRequest object
> have or do not have the prefixes.
> 2) Specify more clearly in (or around) the table which is the default
> policy. Currently the Ignore policy "Behavior" just mentions that it's the
> current behavior, but I think it would help that it is described as the
> default for the property.
>
> Otherwise, this looks good to me.
>
> Best regards,
>
> Randall
>
> On Mon, May 6, 2019 at 8:12 PM Magesh Nandakumar 
> wrote:
>
> > Konstantine,
> >
> > Thanks a lot for your feedback on the KIP. I have incorporated the
> feedback
> > using generics for Class. I have also updated the KIP to handle the
> default
> > value per Randall's suggestion. Let me know if you have any questions.
> >
> > Thanks,
> > Magesh
> >
> >
> > On Mon, May 6, 2019 at 1:58 PM Konstantine Karantasis <
> > konstant...@confluent.io> wrote:
> >
> > > Thanks for the KIP Magesh, it's quite useful towards the goals for more
> > > general multi-tenancy in Connect.
> > >
> > > Couple of comments from me too:
> > >
> > > I think the fact that the default policy is 'null' (no implementation)
> > > should be mentioned on the table next to the available implementations.
> > > Currently the KIP says: 'In addition to the default implementation,
> ..."
> > > but this is not very accurate because there is no concrete default
> > > implementation. Just special handling of 'null' in
> > > 'connector.client.config.policy'
> > >
> > > Regarding passing the overrides to the connector 'configure' method, I
> > feel
> > > it wouldn't hurt to pass them, but I also agree that leaving this out
> at
> > > the moment is the safest option.
> > >
> > > Since the interfaces and classes are listed in the KIP, I'd like to
> note
> > > that Class is used as a raw type in field and return value
> declarations.
> > We
> > > should use the generic type instead.
> > >
> > > Thanks for this improvement proposal!
> > > Konstantine
> > >
> > > On Mon, May 6, 2019 at 11:11 AM Magesh Nandakumar <
> mage...@confluent.io>
> > > wrote:
> > >
> > > > Randall,
> > > >
> > > > I was wondering if you had any thoughts on the above alternatives to
> > deal
> > > > with a default policy.  If it's possible, I would like to finalize
> the
> > > > discussions and start a vote.
> > > > Let me know your thoughts.
> > > >
> > > > Thanks,
> > > > Magesh
> > > >
> > > > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar <
> > mage...@confluent.io>
> > > > wrote:
> > > >
> > > > > Randall,
> > > > >
> > > > > The approach to return the to override configs could possibly make
> it
> > > > > cumbersome to implement a custom policy. This is a new
> configuration
> > > and
> > > > if
> > > > > you don't explicitly set it the existing behavior remains as-is.
> Like
> > > > > Chris, I also preferred this approach for the sake of simplicity.
> If
> > > not
> > > > > for the default `null` I would prefer to fall back to using
> `Ignore`
> > > > which
> > > > > is a misnomer to the interface spec but still gets the job done via
> > > > > instanceOf checks. The other options I could think of are as
> below:-
> > > > >
> > > > >- have an enforcePolicy() method in the interface which by
> default
> > > > >returns true and the Ignore implementation could return false
> > > > >- introduce another worker config
> allow.connector.config.overrides
> > > > >with a default value of false and the default policy can be None
> > > > >
> > > > > Let me know what you think.
> > > > >
> > > > > Thanks
> > > > > Magesh
> > > > >
> > > > > On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch 
> > > wrote:
> > > > >
> > > > >> Than

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi Randall,

I have incorporated your comments and updated the KIP with the following


   1. update the config key to be `connector.client.config.override.policy`
   2. update the interface name to be
   `ConnectorClientConfigOverridePolicy`. Updated the implementation names
   also to match the new interface name
   3. introduce useOverrides() in the interface
   4. make `Ignore` the default implementation for the policy.


Let me know if you have any questions.

Thanks
Magesh

On Mon, May 6, 2019 at 5:45 PM Randall Hauch  wrote:

> That's fine. I don't think there's much difference between the two option
> anyway. I'm looking forward to the updated KIP.
>
> On Mon, May 6, 2019 at 7:38 PM Magesh Nandakumar 
> wrote:
>
> > Randall,
> >
> > Thanks a lot for your feedback.
> >
> > If I understand it correctly, we could do one of the following, right?
> >
> > 1. introduce a new config `allow.client.config.overrides` with a default
> > value of false. The default value for the policy would be `None`. So by
> > default, we will still preserve the current behavior.
> > 2. introduce `useOverride()` default method that returns true in the
> > interface. The `Ignore` policy would then override it to return false.
> > `Ignore` will also be the default policy.
> >
> > I personally prefer option #2, since it involves one less configuration
> but
> > then I'm also open to the other option.
> >
> > Thanks,
> > Magesh
> >
> > On Mon, May 6, 2019 at 5:29 PM Randall Hauch  wrote:
> >
> > > I actually like a separate config for whether to pass or filter client
> > > override properties to the connector. I generally dislike adding more
> > > properties, but in this case it keeps the two orthogonal behaviors
> > > independent and reduces the need to implement policies that cover all
> > > permutations.
> > >
> > > However, I still find it strange to have a "non-policy" be the default.
> > So
> > > either of these modifications to the current KIP would be fine with me:
> > > 1) Add a `useOverride()` default method that returns true, but which
> the
> > > None policy could override and return false; and keep the
> `validate(...)`
> > > method as it is.
> > > 2) Change the `validate(Map<...>) method to support a filtering
> pattern,
> > > such as `Map<...> filterOverrides(Map<...> connectorClientOverrides)`
> > >
> > > The point is that the default is the name of a built-in policy.
> > >
> > > Also, one minor suggestion is to use the term "override" in the config
> > > property (e.g., `connector.client.override.policy`) since that term is
> > used
> > > prevalently and matches the `producer.override`, `consumer.override`,
> and
> > > `admin.override` prefixes.
> > >
> > > Thanks for working through this, Magesh.
> > >
> > > Randall
> > >
> > > On Mon, May 6, 2019 at 1:11 PM Magesh Nandakumar  >
> > > wrote:
> > >
> > > > Randall,
> > > >
> > > > I was wondering if you had any thoughts on the above alternatives to
> > deal
> > > > with a default policy.  If it's possible, I would like to finalize
> the
> > > > discussions and start a vote.
> > > > Let me know your thoughts.
> > > >
> > > > Thanks,
> > > > Magesh
> > > >
> > > > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar <
> > mage...@confluent.io>
> > > > wrote:
> > > >
> > > > > Randall,
> > > > >
> > > > > The approach to return the to override configs could possibly make
> it
> > > > > cumbersome to implement a custom policy. This is a new
> configuration
> > > and
> > > > if
> > > > > you don't explicitly set it the existing behavior remains as-is.
> Like
> > > > > Chris, I also preferred this approach for the sake of simplicity.
> If
> > > not
> > > > > for the default `null` I would prefer to fall back to using
> `Ignore`
> > > > which
> > > > > is a misnomer to the interface spec but still gets the job done via
> > > > > instanceOf checks. The other options I could think of are as
> below:-
> > > > >
> > > > >- have an enforcePolicy() method in the interface which by
> default
> > > > >returns true and the Ignore implementation could return false
> > &

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Konstantine,

Thanks a lot for your feedback on the KIP. I have incorporated the feedback
using generics for Class. I have also updated the KIP to handle the default
value per Randall's suggestion. Let me know if you have any questions.

Thanks,
Magesh


On Mon, May 6, 2019 at 1:58 PM Konstantine Karantasis <
konstant...@confluent.io> wrote:

> Thanks for the KIP Magesh, it's quite useful towards the goals for more
> general multi-tenancy in Connect.
>
> Couple of comments from me too:
>
> I think the fact that the default policy is 'null' (no implementation)
> should be mentioned on the table next to the available implementations.
> Currently the KIP says: 'In addition to the default implementation, ..."
> but this is not very accurate because there is no concrete default
> implementation. Just special handling of 'null' in
> 'connector.client.config.policy'
>
> Regarding passing the overrides to the connector 'configure' method, I feel
> it wouldn't hurt to pass them, but I also agree that leaving this out at
> the moment is the safest option.
>
> Since the interfaces and classes are listed in the KIP, I'd like to note
> that Class is used as a raw type in field and return value declarations. We
> should use the generic type instead.
>
> Thanks for this improvement proposal!
> Konstantine
>
> On Mon, May 6, 2019 at 11:11 AM Magesh Nandakumar 
> wrote:
>
> > Randall,
> >
> > I was wondering if you had any thoughts on the above alternatives to deal
> > with a default policy.  If it's possible, I would like to finalize the
> > discussions and start a vote.
> > Let me know your thoughts.
> >
> > Thanks,
> > Magesh
> >
> > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar 
> > wrote:
> >
> > > Randall,
> > >
> > > The approach to return the to override configs could possibly make it
> > > cumbersome to implement a custom policy. This is a new configuration
> and
> > if
> > > you don't explicitly set it the existing behavior remains as-is. Like
> > > Chris, I also preferred this approach for the sake of simplicity.  If
> not
> > > for the default `null` I would prefer to fall back to using `Ignore`
> > which
> > > is a misnomer to the interface spec but still gets the job done via
> > > instanceOf checks. The other options I could think of are as below:-
> > >
> > >- have an enforcePolicy() method in the interface which by default
> > >returns true and the Ignore implementation could return false
> > >- introduce another worker config allow.connector.config.overrides
> > >with a default value of false and the default policy can be None
> > >
> > > Let me know what you think.
> > >
> > > Thanks
> > > Magesh
> > >
> > > On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch 
> wrote:
> > >
> > >> Thanks, Chris. I still think it's strange to have a non-policy, since
> > >> there's now special behavior for when the policy is not specified.
> > >>
> > >> Perhaps the inability for a policy implementation to represent the
> > >> existing
> > >> behavior suggests that the policy interface isn't quite right. Could
> the
> > >> policy's "validate" method take the overrides that were supplied and
> > >> return
> > >> the overrides that should be passed to the connector, yet still
> throwing
> > >> an
> > >> exception if any supplied overrides are not allowed. Then the
> different
> > >> policy implementations might be:
> > >>
> > >>- Ignore (default) - returns all supplied override properties
> > >>- None - throws exception if any override properties are supplied;
> > >>always returns empty map if no overrides are provided
> > >>- Principal - throws exception if other override properties are
> > >>provided, but returns an empty map (since no properties should be
> > >> passed to
> > >>the connector)
> > >>- All - returns all provided override properties
> > >>
> > >> All override properties defined on the connector configuration would
> be
> > >> passed to the policy for validation, and assuming there's no error all
> > of
> > >> these overrides would be used in the producer/consumer/admin client.
> The
> > >> result of the policy call, however, is used to determine which of
> these
> > >> overrides are passed to the connector.
> > >>

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Randall,

Thanks a lot for your feedback.

If I understand it correctly, we could do one of the following, right?

1. introduce a new config `allow.client.config.overrides` with a default
value of false. The default value for the policy would be `None`. So by
default, we will still preserve the current behavior.
2. introduce `useOverride()` default method that returns true in the
interface. The `Ignore` policy would then override it to return false.
`Ignore` will also be the default policy.

I personally prefer option #2, since it involves one less configuration but
then I'm also open to the other option.

Thanks,
Magesh

On Mon, May 6, 2019 at 5:29 PM Randall Hauch  wrote:

> I actually like a separate config for whether to pass or filter client
> override properties to the connector. I generally dislike adding more
> properties, but in this case it keeps the two orthogonal behaviors
> independent and reduces the need to implement policies that cover all
> permutations.
>
> However, I still find it strange to have a "non-policy" be the default. So
> either of these modifications to the current KIP would be fine with me:
> 1) Add a `useOverride()` default method that returns true, but which the
> None policy could override and return false; and keep the `validate(...)`
> method as it is.
> 2) Change the `validate(Map<...>) method to support a filtering pattern,
> such as `Map<...> filterOverrides(Map<...> connectorClientOverrides)`
>
> The point is that the default is the name of a built-in policy.
>
> Also, one minor suggestion is to use the term "override" in the config
> property (e.g., `connector.client.override.policy`) since that term is used
> prevalently and matches the `producer.override`, `consumer.override`, and
> `admin.override` prefixes.
>
> Thanks for working through this, Magesh.
>
> Randall
>
> On Mon, May 6, 2019 at 1:11 PM Magesh Nandakumar 
> wrote:
>
> > Randall,
> >
> > I was wondering if you had any thoughts on the above alternatives to deal
> > with a default policy.  If it's possible, I would like to finalize the
> > discussions and start a vote.
> > Let me know your thoughts.
> >
> > Thanks,
> > Magesh
> >
> > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar 
> > wrote:
> >
> > > Randall,
> > >
> > > The approach to return the to override configs could possibly make it
> > > cumbersome to implement a custom policy. This is a new configuration
> and
> > if
> > > you don't explicitly set it the existing behavior remains as-is. Like
> > > Chris, I also preferred this approach for the sake of simplicity.  If
> not
> > > for the default `null` I would prefer to fall back to using `Ignore`
> > which
> > > is a misnomer to the interface spec but still gets the job done via
> > > instanceOf checks. The other options I could think of are as below:-
> > >
> > >- have an enforcePolicy() method in the interface which by default
> > >returns true and the Ignore implementation could return false
> > >- introduce another worker config allow.connector.config.overrides
> > >with a default value of false and the default policy can be None
> > >
> > > Let me know what you think.
> > >
> > > Thanks
> > > Magesh
> > >
> > > On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch 
> wrote:
> > >
> > >> Thanks, Chris. I still think it's strange to have a non-policy, since
> > >> there's now special behavior for when the policy is not specified.
> > >>
> > >> Perhaps the inability for a policy implementation to represent the
> > >> existing
> > >> behavior suggests that the policy interface isn't quite right. Could
> the
> > >> policy's "validate" method take the overrides that were supplied and
> > >> return
> > >> the overrides that should be passed to the connector, yet still
> throwing
> > >> an
> > >> exception if any supplied overrides are not allowed. Then the
> different
> > >> policy implementations might be:
> > >>
> > >>- Ignore (default) - returns all supplied override properties
> > >>- None - throws exception if any override properties are supplied;
> > >>always returns empty map if no overrides are provided
> > >>- Principal - throws exception if other override properties are
> > >>provided, but returns an empty map (since no properties should be
> > >> passed to
> > >>the connector)
> &g

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Randall,

I was wondering if you had any thoughts on the above alternatives to deal
with a default policy.  If it's possible, I would like to finalize the
discussions and start a vote.
Let me know your thoughts.

Thanks,
Magesh

On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar 
wrote:

> Randall,
>
> The approach to return the to override configs could possibly make it
> cumbersome to implement a custom policy. This is a new configuration and if
> you don't explicitly set it the existing behavior remains as-is. Like
> Chris, I also preferred this approach for the sake of simplicity.  If not
> for the default `null` I would prefer to fall back to using `Ignore` which
> is a misnomer to the interface spec but still gets the job done via
> instanceOf checks. The other options I could think of are as below:-
>
>- have an enforcePolicy() method in the interface which by default
>returns true and the Ignore implementation could return false
>- introduce another worker config allow.connector.config.overrides
>with a default value of false and the default policy can be None
>
> Let me know what you think.
>
> Thanks
> Magesh
>
> On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch  wrote:
>
>> Thanks, Chris. I still think it's strange to have a non-policy, since
>> there's now special behavior for when the policy is not specified.
>>
>> Perhaps the inability for a policy implementation to represent the
>> existing
>> behavior suggests that the policy interface isn't quite right. Could the
>> policy's "validate" method take the overrides that were supplied and
>> return
>> the overrides that should be passed to the connector, yet still throwing
>> an
>> exception if any supplied overrides are not allowed. Then the different
>> policy implementations might be:
>>
>>- Ignore (default) - returns all supplied override properties
>>- None - throws exception if any override properties are supplied;
>>always returns empty map if no overrides are provided
>>- Principal - throws exception if other override properties are
>>provided, but returns an empty map (since no properties should be
>> passed to
>>the connector)
>>- All - returns all provided override properties
>>
>> All override properties defined on the connector configuration would be
>> passed to the policy for validation, and assuming there's no error all of
>> these overrides would be used in the producer/consumer/admin client. The
>> result of the policy call, however, is used to determine which of these
>> overrides are passed to the connector.
>>
>> This approach means that all behaviors can be implemented through a policy
>> class, including the defaults. It also gives a bit more control to custom
>> policies, should that be warranted. For example, validating the provided
>> client overrides but passing all such override properties to the
>> connector,
>> which as I stated earlier is something I think connectors likely don't
>> look
>> for.
>>
>> Thoughts?
>>
>> Randall
>>
>> On Tue, Apr 30, 2019 at 6:07 PM Chris Egerton 
>> wrote:
>>
>> > Randall,
>> >
>> > The special behavior for null was my suggestion. There is no
>> implementation
>> > of the proposed interface that causes client overrides to be ignored, so
>> > the original idea was to have a special implementation that would be
>> > checked for by the Connect framework (probably via the instanceof
>> operator)
>> > and, if present, cause all would-be overrides to be ignored.
>> >
>> > I thought this may be confusing to people who may see that behavior and
>> > wonder how to recreate it themselves, so I suggested leaving that policy
>> > out and replace it with a check to see if a policy was specified at all.
>> >
>> > Would be interested in your thoughts on this, especially if there's an
>> > alternative that hasn't been proposed yet.
>> >
>> > Cheers,
>> >
>> > Chris
>> >
>> > On Tue, Apr 30, 2019, 18:01 Randall Hauch  wrote:
>> >
>> > > On Tue, Apr 30, 2019 at 4:20 PM Magesh Nandakumar <
>> mage...@confluent.io>
>> > > wrote:
>> > >
>> > > > Randall,
>> > > >
>> > > > Thanks a lot for your feedback.
>> > > >
>> > > > You bring up an interesting point regarding the overrides being
>> > available
>> > > > to the connectors. Today everything that is specified in the config
>

Re: [VOTE] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

Thanks a lot for the work on this KIP Chris.

+1(non-binding)

On Thu, May 2, 2019, 4:56 PM Chris Egerton  wrote:

> Hi all,
>
> I'd like to start a vote for KIP-454:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-454%3A+Expansion+of+the+ConnectClusterState+interface
>
> The discussion thread can be found at
> https://www.mail-archive.com/dev@kafka.apache.org/msg96911.html
>
> Thanks to Konstantine Karantasis and Magesh Nandakumar for their thoughtful
> feedback!
>
> Cheers,
>
> Chris
>

Re: [VOT] KIP-449: Add connector contexts to Connect worker logs (vote thread)

[Magesh Nandakumar]

This will make connect debugging so much easier. Thanks a lot for driving
this Randall.

+1 (non-binding)

Thanks,
Magesh

On Tue, Apr 30, 2019 at 7:19 PM Jeremy Custenborder 
wrote:

> +1 non binding
>
> On Mon, Apr 29, 2019 at 5:34 PM Randall Hauch  wrote:
> >
> > I would like to start the vote for KIP-258:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-449%3A+Add+connector+contexts+to+Connect+worker+logs
> >
> > The KIP uses the Mapped Diagnostic Contexts (MDC) feature of SLF4J API to
> > add more context to log messages from within Connect workers and
> connector
> > implementations. This would not be enabled by default, though it would be
> > easy to enable within the Connect Log4J configuration.
> >
> > Thanks!
> >
> > Randall
>

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Randall,

The approach to return the to override configs could possibly make it
cumbersome to implement a custom policy. This is a new configuration and if
you don't explicitly set it the existing behavior remains as-is. Like
Chris, I also preferred this approach for the sake of simplicity.  If not
for the default `null` I would prefer to fall back to using `Ignore` which
is a misnomer to the interface spec but still gets the job done via
instanceOf checks. The other options I could think of are as below:-

   - have an enforcePolicy() method in the interface which by default
   returns true and the Ignore implementation could return false
   - introduce another worker config allow.connector.config.overrides with
   a default value of false and the default policy can be None

Let me know what you think.

Thanks
Magesh

On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch  wrote:

> Thanks, Chris. I still think it's strange to have a non-policy, since
> there's now special behavior for when the policy is not specified.
>
> Perhaps the inability for a policy implementation to represent the existing
> behavior suggests that the policy interface isn't quite right. Could the
> policy's "validate" method take the overrides that were supplied and return
> the overrides that should be passed to the connector, yet still throwing an
> exception if any supplied overrides are not allowed. Then the different
> policy implementations might be:
>
>- Ignore (default) - returns all supplied override properties
>- None - throws exception if any override properties are supplied;
>always returns empty map if no overrides are provided
>- Principal - throws exception if other override properties are
>provided, but returns an empty map (since no properties should be
> passed to
>the connector)
>- All - returns all provided override properties
>
> All override properties defined on the connector configuration would be
> passed to the policy for validation, and assuming there's no error all of
> these overrides would be used in the producer/consumer/admin client. The
> result of the policy call, however, is used to determine which of these
> overrides are passed to the connector.
>
> This approach means that all behaviors can be implemented through a policy
> class, including the defaults. It also gives a bit more control to custom
> policies, should that be warranted. For example, validating the provided
> client overrides but passing all such override properties to the connector,
> which as I stated earlier is something I think connectors likely don't look
> for.
>
> Thoughts?
>
> Randall
>
> On Tue, Apr 30, 2019 at 6:07 PM Chris Egerton  wrote:
>
> > Randall,
> >
> > The special behavior for null was my suggestion. There is no
> implementation
> > of the proposed interface that causes client overrides to be ignored, so
> > the original idea was to have a special implementation that would be
> > checked for by the Connect framework (probably via the instanceof
> operator)
> > and, if present, cause all would-be overrides to be ignored.
> >
> > I thought this may be confusing to people who may see that behavior and
> > wonder how to recreate it themselves, so I suggested leaving that policy
> > out and replace it with a check to see if a policy was specified at all.
> >
> > Would be interested in your thoughts on this, especially if there's an
> > alternative that hasn't been proposed yet.
> >
> > Cheers,
> >
> > Chris
> >
> > On Tue, Apr 30, 2019, 18:01 Randall Hauch  wrote:
> >
> > > On Tue, Apr 30, 2019 at 4:20 PM Magesh Nandakumar <
> mage...@confluent.io>
> > > wrote:
> > >
> > > > Randall,
> > > >
> > > > Thanks a lot for your feedback.
> > > >
> > > > You bring up an interesting point regarding the overrides being
> > available
> > > > to the connectors. Today everything that is specified in the config
> > while
> > > > creating is available for the connector. But this is a specific case
> > and
> > > we
> > > > could do either of the following
> > > >
> > > >
> > > >- don't pass any configs with these prefixes to the
> ConnectorConfig
> > > >instance that's passed in the startConnector
> > > >- allow policies as to whether the configurations with the
> prefixes
> > > >should be made available to the connector or not. Should this also
> > > > define a
> > > >list of configurations?
> > > >
> > > > I personally prefer not passing the configs to 

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Randall,

Thanks a lot for your feedback.

You bring up an interesting point regarding the overrides being available
to the connectors. Today everything that is specified in the config while
creating is available for the connector. But this is a specific case and we
could do either of the following


   - don't pass any configs with these prefixes to the ConnectorConfig
   instance that's passed in the startConnector
   - allow policies as to whether the configurations with the prefixes
   should be made available to the connector or not. Should this also define a
   list of configurations?

I personally prefer not passing the configs to Connector since that's
simple, straight forward and don't see a reason for the connector to access
those.

For the second point,  None - doesn't allow overrides and the default
policy is null. We preserve backward compatibility when no policy is
configured. Let me know if that's not clear in the KIP.

Thanks,
Magesh

On Mon, Apr 29, 2019 at 4:07 PM Randall Hauch  wrote:

> Per the proposal, a connector configuration can define one or more
> properties that begin with any of the three prefixes: "producer.override.",
> "consumer.override.", and "admin.override.". The proposal states:
>
> Since the users can specify any of these policies, the connectors itself
> should not rely on these configurations to be available. The overrides are
> to be used purely from an operational perspective.
>
>
> Does this mean that any such properties are visible to connectors, or will
> they be hidden to connectors? Currently no connectors have access to such
> client properties, and users are unlike to just put them into a connector
> configuration unnecessarily. A connector implementation could have defined
> such properties as normal connector-specific properties, in which case they
> are required, but is that likely given the log prefixes? One concern that I
> have is that this might allow connector implementations start attempting to
> circumvent the Connect API if these properties are included.
>
> Second, does the None policy allow but ignore these additional properties
> (e.g., "validate(...)" is simply a no-op)? Or does the None policy fail if
> any client overrides are specified? The former seems more in line with the
> current behavior, whereas the "disallows" policy seems useful but not
> exactly backward compatible. Should we also offer a "Disallow" policy? In
> fact, should the policies be named "Ignore" (default), "Disallow",
> "Prinicipal", and "All"?
>
> Otherwise, I like the idea of this. There have been several requests over
> the past year or two for adding subsets of this functionality. Might be
> good to find and list all of the related KAFKA issues.
>
> Randall
>
> On Fri, Apr 26, 2019 at 4:04 PM Chris Egerton  wrote:
>
> > Hi Magesh,
> >
> > Changes look good to me! Excited to see this happen, hope the KIP passes
> :)
> >
> > Cheers,
> >
> > Chris
> >
> > On Fri, Apr 26, 2019 at 1:44 PM Magesh Nandakumar 
> > wrote:
> >
> > > Hi Chris,
> > >
> > > I have updated the KIP to reflect the changes that we discussed for the
> > > prefix. Thanks for all your inputs.
> > >
> > > Thanks,
> > > Magesh
> > >
> > > On Thu, Apr 25, 2019 at 2:18 PM Chris Egerton 
> > wrote:
> > >
> > > > Hi Magesh,
> > > >
> > > > Agreed that we should avoid `dlq.admin`. I also don't have a strong
> > > opinion
> > > > between `connector.` and `.override`, but I have a slight inclination
> > > > toward `.override` since `connector.` feels a little redundant given
> > that
> > > > the whole configuration is for the connector and the use of
> "override"
> > > may
> > > > shed a little light on how the properties for these clients are
> > computed
> > > > and help make the learning curve a little gentler on new devs and
> > users.
> > > >
> > > > Regardless, I think the larger issue of conflicts with existing
> > > properties
> > > > (both in MM2 and potentially other connectors) has been
> satisfactorily
> > > > addressed, so I'm happy.
> > > >
> > > > Cheers,
> > > >
> > > > Chris
> > > >
> > > > On Wed, Apr 24, 2019 at 11:14 AM Magesh Nandakumar <
> > mage...@confluent.io
> > > >
> > > > wrote:
> > > >
> > > > > HI Chrise,
> > > > >
> > > > > You are right about the "admin

Re: [VOTE] KIP-411: Make default Kafka Connect worker task client IDs distinct

[Magesh Nandakumar]

Looks good to me and a very useful feature.

+1 ( non-binding)

On Mon, Apr 29, 2019, 4:05 PM Arjun Satish  wrote:

> Thanks, Paul! This is very useful.
>
> +1 (non-binding)
>
> Best,
> Arjun
>
> On Fri, Apr 12, 2019 at 4:13 PM Ryanne Dolan 
> wrote:
>
> > +1 (non binding)
> >
> > Thanks
> > Ryanne
> >
> > On Fri, Apr 12, 2019, 11:11 AM Paul Davidson
> >  wrote:
> >
> > > Just a reminder that KIP-411 is open for voting. No votes received yet!
> > >
> > > On Fri, Apr 5, 2019 at 9:07 AM Paul Davidson  >
> > > wrote:
> > >
> > > > Hi all,
> > > >
> > > > Since we seem to have agreement in the discussion I would like to
> start
> > > > the vote on KIP-411.
> > > >
> > > > See:
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-411%3A+Make+default+Kafka+Connect+worker+task+client+IDs+distinct
> > > >
> > > > Also see the related PR: https://github.com/apache/kafka/pull/6097
> > > >
> > > > Thanks to everyone who contributed!
> > > >
> > > > Paul
> > > >
> > >
> >
>

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi Chris,

I have updated the KIP to reflect the changes that we discussed for the
prefix. Thanks for all your inputs.

Thanks,
Magesh

On Thu, Apr 25, 2019 at 2:18 PM Chris Egerton  wrote:

> Hi Magesh,
>
> Agreed that we should avoid `dlq.admin`. I also don't have a strong opinion
> between `connector.` and `.override`, but I have a slight inclination
> toward `.override` since `connector.` feels a little redundant given that
> the whole configuration is for the connector and the use of "override" may
> shed a little light on how the properties for these clients are computed
> and help make the learning curve a little gentler on new devs and users.
>
> Regardless, I think the larger issue of conflicts with existing properties
> (both in MM2 and potentially other connectors) has been satisfactorily
> addressed, so I'm happy.
>
> Cheers,
>
> Chris
>
> On Wed, Apr 24, 2019 at 11:14 AM Magesh Nandakumar 
> wrote:
>
> > HI Chrise,
> >
> > You are right about the "admin." prefix creating conflicts. Here are few
> > options that I can think of
> >
> > 1. Use `dlq.admin` since admin client is used only for DLQ. But this
> might
> > not really be the case in the future. So, we should possibly drop this
> idea
> > :)
> > 2.  Use `connector.producer`, `connector.consumer` and `connector.admin`
> -
> > provides better context that its connector specific property
> > 3.  Use `producer.override`, '`consumer.override` and `admin.override` -
> > provides better clarity that these are overrides.
> >
> > I don't have a strong opinion in choosing between #2 and #3. Let me
> > know what you think.
> >
> > Thanks
> > Magesh
> >
> > On Wed, Apr 24, 2019 at 10:25 AM Chris Egerton 
> > wrote:
> >
> > > Hi Magesh,
> > >
> > > Next round :)
> > >
> > > 1. It looks like MM2 will also support "admin." properties that affect
> > > AdminClients it creates and uses, which IIUC is the same prefix name to
> > be
> > > used for managing the DLQ for sink connectors in this KIP. Doesn't that
> > > still leave room for conflict? I'm imagining a scenario like this: a
> > > Connect worker is configured to use the
> > > PrincipalConnectorClientConfigPolicy, someone tries to start an
> instance
> > of
> > > an MM2 sink with "admin." properties beyond just
> > "admin.sasl.jaas.config",
> > > and gets rejected because those properties are then interpreted by the
> > > worker as overrides for the AdminClient it uses to manage the DLQ.
> > > 2. (LGTM)
> > > 3. I'm convinced by this, as long as nobody else identifies a common
> use
> > > case that would involve a similar client config policy implementation
> > that
> > > would be limited to a small set of whitelisted configs. For now keeping
> > the
> > > PrincipalConnectorClientConfigPolicy sounds fine to me.
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Tue, Apr 23, 2019 at 10:30 PM Magesh Nandakumar <
> mage...@confluent.io
> > >
> > > wrote:
> > >
> > > > Hi all,
> > > >
> > > > I also have a draft implementation of the KIP
> > > > https://github.com/apache/kafka/pull/6624. I would still need to
> > include
> > > > more tests and docs but I thought it would be useful to have this for
> > the
> > > > KIP discussion. Looking forward to all of your valuable feedback.
> > > >
> > > > Thanks
> > > > Magesh
> > > >
> > > > On Tue, Apr 23, 2019 at 10:27 PM Magesh Nandakumar <
> > mage...@confluent.io
> > > >
> > > > wrote:
> > > >
> > > > > Chrise,
> > > > >
> > > > > Thanks a lot for your feedback. I will address them in order of
> your
> > > > > questions/comments.
> > > > >
> > > > > 1. Thanks for bringing this to my attention about KIP-382. I had a
> > > closer
> > > > > look at the KIP and IIUC, the KIP allows `consumer.` prefix for
> > > > SourceConnector
> > > > > and producer. prefix for SinkConnector since those are additional
> > > > > connector properties to help resolve the Kafka cluster other than
> the
> > > one
> > > > > Connect framework knows about. Whereas, the proposal in KIP-458
> > applies
> > > > > producer policies for SinkConnectors and consumer policies
&

Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

Thanks a lot, Chris. The KIP looks good to me.

On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton  wrote:

> Hi Magesh,
>
> Sounds good; I've updated the KIP to make ConnectClusterDetails an
> interface. If we want to leave the door open to expand it in the future it
> definitely makes sense to treat it similarly to how we're treating the
> ConnectClusterState interface now.
>
> Thanks,
>
> Chris
>
> On Thu, Apr 25, 2019 at 4:18 PM Magesh Nandakumar 
> wrote:
>
> > HI Chrise,
> >
> > Overall it looks good to me. Just one last comment - I was wondering if
> > ConnectClusterDetail should be an interface just like
> ConnectClusterState.
> >
> > Thanks,
> > Magesh
> >
> > On Thu, Apr 25, 2019 at 3:54 PM Chris Egerton 
> wrote:
> >
> > > Hi Magesh,
> > >
> > > Expanding the type we use to convey cluster metadata from just a Kafka
> > > cluster ID string to its own class seems like a good idea for the sake
> of
> > > forwards compatibility, but I'm still not sure what the gains of
> > including
> > > the cluster group ID would be--it's a simple map lookup away in the
> REST
> > > extension's configure(...) method. Including information on whether the
> > > cluster is distributed or standalone definitely seems convenient; as
> far
> > as
> > > I can tell there's no easy way to do that from within a REST extension
> at
> > > the moment, and relying on something like the presence of a group.id
> > > property to identify a distributed cluster could result in false
> > positives.
> > > However, is there a use case for it? If not, we can note that as a
> > possible
> > > addition to the ConnectClusterDetails class for later but leave it out
> > for
> > > now.
> > >
> > > I've updated the KIP to include the new ConnectClusterDetails class but
> > > left out the cluster type information for now; let me know what you
> > think.
> > >
> > > Thanks again for your thoughts!
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Wed, Apr 24, 2019 at 4:49 PM Magesh Nandakumar <
> mage...@confluent.io>
> > > wrote:
> > >
> > > > Hi Chris,
> > > >
> > > > Instead of calling it ConnectClusterId, perhaps call it
> > > > ConnectClusterDetails which can include things like groupid,
> underlying
> > > > kafkaclusterId, standalone or distributed, etc. This will help expose
> > any
> > > > cluster related information in the future.
> > > > Let me know if that would work?
> > > >
> > > > Thanks,
> > > > Magesh
> > > >
> > > > On Wed, Apr 24, 2019 at 4:26 PM Chris Egerton 
> > > wrote:
> > > >
> > > > > Hi Magesh,
> > > > >
> > > > > 1. After ruminating for a little while on the inclusion of a method
> > to
> > > > > retrieve task configurations I've tentatively decided to remove it
> > from
> > > > the
> > > > > proposal and place it in the rejected alternatives section. If
> anyone
> > > > > presents a reasonable use case for it I'll be happy to discuss
> > further
> > > > but
> > > > > right now I think this is the way to go. Thanks for your
> suggestion!
> > > > >
> > > > > 2. The idea of a Connect cluster ID method is certainly
> fascinating,
> > > but
> > > > > there are a few questions it raises. First off, what would the
> > > group.id
> > > > be
> > > > > for a standalone cluster? Second, why return a formatted string
> there
> > > > > instead of a new class such as a ConnectClusterId that provides the
> > two
> > > > in
> > > > > separate methods? And lastly, since REST extensions are configured
> > with
> > > > all
> > > > > of the properties available to the worker, wouldn't it be possible
> to
> > > > just
> > > > > get the group ID of the Connect cluster from there? The reason I'd
> > like
> > > > to
> > > > > see the Kafka cluster ID made available to REST extensions is that
> > > > > retrieving it isn't as simple as reading a configuration from a
> > > > properties
> > > > > map and instead involves creating an admin client from those
> > properties
> > > > and
> > > > > using it

Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

HI Chrise,

Overall it looks good to me. Just one last comment - I was wondering if
ConnectClusterDetail should be an interface just like ConnectClusterState.

Thanks,
Magesh

On Thu, Apr 25, 2019 at 3:54 PM Chris Egerton  wrote:

> Hi Magesh,
>
> Expanding the type we use to convey cluster metadata from just a Kafka
> cluster ID string to its own class seems like a good idea for the sake of
> forwards compatibility, but I'm still not sure what the gains of including
> the cluster group ID would be--it's a simple map lookup away in the REST
> extension's configure(...) method. Including information on whether the
> cluster is distributed or standalone definitely seems convenient; as far as
> I can tell there's no easy way to do that from within a REST extension at
> the moment, and relying on something like the presence of a group.id
> property to identify a distributed cluster could result in false positives.
> However, is there a use case for it? If not, we can note that as a possible
> addition to the ConnectClusterDetails class for later but leave it out for
> now.
>
> I've updated the KIP to include the new ConnectClusterDetails class but
> left out the cluster type information for now; let me know what you think.
>
> Thanks again for your thoughts!
>
> Cheers,
>
> Chris
>
> On Wed, Apr 24, 2019 at 4:49 PM Magesh Nandakumar 
> wrote:
>
> > Hi Chris,
> >
> > Instead of calling it ConnectClusterId, perhaps call it
> > ConnectClusterDetails which can include things like groupid, underlying
> > kafkaclusterId, standalone or distributed, etc. This will help expose any
> > cluster related information in the future.
> > Let me know if that would work?
> >
> > Thanks,
> > Magesh
> >
> > On Wed, Apr 24, 2019 at 4:26 PM Chris Egerton 
> wrote:
> >
> > > Hi Magesh,
> > >
> > > 1. After ruminating for a little while on the inclusion of a method to
> > > retrieve task configurations I've tentatively decided to remove it from
> > the
> > > proposal and place it in the rejected alternatives section. If anyone
> > > presents a reasonable use case for it I'll be happy to discuss further
> > but
> > > right now I think this is the way to go. Thanks for your suggestion!
> > >
> > > 2. The idea of a Connect cluster ID method is certainly fascinating,
> but
> > > there are a few questions it raises. First off, what would the
> group.id
> > be
> > > for a standalone cluster? Second, why return a formatted string there
> > > instead of a new class such as a ConnectClusterId that provides the two
> > in
> > > separate methods? And lastly, since REST extensions are configured with
> > all
> > > of the properties available to the worker, wouldn't it be possible to
> > just
> > > get the group ID of the Connect cluster from there? The reason I'd like
> > to
> > > see the Kafka cluster ID made available to REST extensions is that
> > > retrieving it isn't as simple as reading a configuration from a
> > properties
> > > map and instead involves creating an admin client from those properties
> > and
> > > using it to perform a `describe cluster` call, which comes with its own
> > > pitfalls as far as error handling, interruptions, and timeouts go.
> Since
> > > this information is available to the herder already, it seems like a
> good
> > > tradeoff to expose that information to REST extensions so that
> developers
> > > don't have to duplicate that logic themselves. I'm unsure that the same
> > > arguments would apply to exposing a group.id to REST extensions
> through
> > > the
> > > ConnectClusterInterface. What do you think?
> > >
> > > Thanks again for your thoughts!
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Tue, Apr 23, 2019 at 4:18 PM Magesh Nandakumar <
> mage...@confluent.io>
> > > wrote:
> > >
> > > > Chris,
> > > >
> > > > I certainly would love to hear others thoughts on #1 but IMO, it
> might
> > > not
> > > > be as useful as ConnectorConfigs and as you mentioned, we could
> always
> > > add
> > > > it when the need arises.
> > > > Thanks for clarifying the details on my concern #2 regarding the
> > > > kafkaClusterId. While not a perfect fit in the interface, I'm not
> > > > completely opposed to having it in the interface. The other option, I
> > can
> > > > think is to expose a connectClusterId() returning grou

Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

Hi Chris,

Instead of calling it ConnectClusterId, perhaps call it
ConnectClusterDetails which can include things like groupid, underlying
kafkaclusterId, standalone or distributed, etc. This will help expose any
cluster related information in the future.
Let me know if that would work?

Thanks,
Magesh

On Wed, Apr 24, 2019 at 4:26 PM Chris Egerton  wrote:

> Hi Magesh,
>
> 1. After ruminating for a little while on the inclusion of a method to
> retrieve task configurations I've tentatively decided to remove it from the
> proposal and place it in the rejected alternatives section. If anyone
> presents a reasonable use case for it I'll be happy to discuss further but
> right now I think this is the way to go. Thanks for your suggestion!
>
> 2. The idea of a Connect cluster ID method is certainly fascinating, but
> there are a few questions it raises. First off, what would the group.id be
> for a standalone cluster? Second, why return a formatted string there
> instead of a new class such as a ConnectClusterId that provides the two in
> separate methods? And lastly, since REST extensions are configured with all
> of the properties available to the worker, wouldn't it be possible to just
> get the group ID of the Connect cluster from there? The reason I'd like to
> see the Kafka cluster ID made available to REST extensions is that
> retrieving it isn't as simple as reading a configuration from a properties
> map and instead involves creating an admin client from those properties and
> using it to perform a `describe cluster` call, which comes with its own
> pitfalls as far as error handling, interruptions, and timeouts go. Since
> this information is available to the herder already, it seems like a good
> tradeoff to expose that information to REST extensions so that developers
> don't have to duplicate that logic themselves. I'm unsure that the same
> arguments would apply to exposing a group.id to REST extensions through
> the
> ConnectClusterInterface. What do you think?
>
> Thanks again for your thoughts!
>
> Cheers,
>
> Chris
>
> On Tue, Apr 23, 2019 at 4:18 PM Magesh Nandakumar 
> wrote:
>
> > Chris,
> >
> > I certainly would love to hear others thoughts on #1 but IMO, it might
> not
> > be as useful as ConnectorConfigs and as you mentioned, we could always
> add
> > it when the need arises.
> > Thanks for clarifying the details on my concern #2 regarding the
> > kafkaClusterId. While not a perfect fit in the interface, I'm not
> > completely opposed to having it in the interface. The other option, I can
> > think is to expose a connectClusterId() returning group.id +
> > kafkaClusterId
> > (with some delimiter) rather than returning the kafkaClusterId. If we
> > choose to go this route, we can even make this a first-class citizen of
> the
> > Herder interface. Let me know what you think.
> >
> > Thanks
> > Magesh
> >
> > On Thu, Apr 18, 2019 at 2:45 PM Chris Egerton 
> wrote:
> >
> > > Hi Magesh,
> > >
> > > Thanks for your comments. I'll address them in the order you provided
> > them:
> > >
> > > 1 - Reason for exposing task configurations to REST extensions:
> > > Yes, the motivation is a little thin for exposing task configs to REST
> > > extensions. I can think of a few uses for this functionality, such as
> > > attempting to infer problematic configurations by examining failed
> tasks
> > > and comparing their configurations to the configurations of running
> > tasks,
> > > but like you've indicated it's dubious that the best place for anything
> > > like that belongs in a REST extension.
> > > I'd be interested to hear others' thoughts, but right now I'm not too
> > > opposed to erring on the side of caution and leaving it out. Worst
> case,
> > it
> > > takes another KIP to add this later on down the road, but that's a
> small
> > > price to pay to avoid adding support for a feature that nobody needs.
> > >
> > > 2. Usefulness of exposing Kafka cluster ID to REST extensions:
> > > As the KIP states, "the Kafka cluster ID may be useful for the purpose
> of
> > > uniquely identifying a Connect cluster from within a REST extension,
> > since
> > > users may be running multiple Kafka clusters and the group.id for a
> > > distributed Connect cluster may not be sufficient to identify a
> cluster."
> > > Even though there may be producer or consumer overrides for
> > > bootstrap.servers present in the configuration for the worker, these
> will
> > > not affect which Kafka cluster is used as a backing sto

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

HI Chrise,

You are right about the "admin." prefix creating conflicts. Here are few
options that I can think of

1. Use `dlq.admin` since admin client is used only for DLQ. But this might
not really be the case in the future. So, we should possibly drop this idea
:)
2.  Use `connector.producer`, `connector.consumer` and `connector.admin` -
provides better context that its connector specific property
3.  Use `producer.override`, '`consumer.override` and `admin.override` -
provides better clarity that these are overrides.

I don't have a strong opinion in choosing between #2 and #3. Let me
know what you think.

Thanks
Magesh

On Wed, Apr 24, 2019 at 10:25 AM Chris Egerton  wrote:

> Hi Magesh,
>
> Next round :)
>
> 1. It looks like MM2 will also support "admin." properties that affect
> AdminClients it creates and uses, which IIUC is the same prefix name to be
> used for managing the DLQ for sink connectors in this KIP. Doesn't that
> still leave room for conflict? I'm imagining a scenario like this: a
> Connect worker is configured to use the
> PrincipalConnectorClientConfigPolicy, someone tries to start an instance of
> an MM2 sink with "admin." properties beyond just "admin.sasl.jaas.config",
> and gets rejected because those properties are then interpreted by the
> worker as overrides for the AdminClient it uses to manage the DLQ.
> 2. (LGTM)
> 3. I'm convinced by this, as long as nobody else identifies a common use
> case that would involve a similar client config policy implementation that
> would be limited to a small set of whitelisted configs. For now keeping the
> PrincipalConnectorClientConfigPolicy sounds fine to me.
>
> Cheers,
>
> Chris
>
> On Tue, Apr 23, 2019 at 10:30 PM Magesh Nandakumar 
> wrote:
>
> > Hi all,
> >
> > I also have a draft implementation of the KIP
> > https://github.com/apache/kafka/pull/6624. I would still need to include
> > more tests and docs but I thought it would be useful to have this for the
> > KIP discussion. Looking forward to all of your valuable feedback.
> >
> > Thanks
> > Magesh
> >
> > On Tue, Apr 23, 2019 at 10:27 PM Magesh Nandakumar  >
> > wrote:
> >
> > > Chrise,
> > >
> > > Thanks a lot for your feedback. I will address them in order of your
> > > questions/comments.
> > >
> > > 1. Thanks for bringing this to my attention about KIP-382. I had a
> closer
> > > look at the KIP and IIUC, the KIP allows `consumer.` prefix for
> > SourceConnector
> > > and producer. prefix for SinkConnector since those are additional
> > > connector properties to help resolve the Kafka cluster other than the
> one
> > > Connect framework knows about. Whereas, the proposal in KIP-458 applies
> > > producer policies for SinkConnectors and consumer policies
> > > SourceConnectors.  So, from what I understand this new policy should
> work
> > > without any issues even for Mirror Maker 2.0.
> > > 2. I have updated the KIP to use a default value of null and use that
> to
> > > determine if we need to ignore overrides.
> > > 3. I would still prefer to keep the special
> > PrincipalConnectorClientConfigPolicy
> > > since that is one of the most common use cases one would choose to use
> > this
> > > feature. If we make it a general case, that would involve users
> requiring
> > > to add additional configuration and they might require well more than
> > just
> > > the list of configs but might also want some restriction on values. If
> > the
> > > concern is about users wanting principal and also other configs, it
> would
> > > still be possible by means of a custom implementation. As is, I would
> > > prefer to keep the proposal to be the same for this. Let me know your
> > > thoughts.
> > >
> > > Thanks,
> > > Magesh
> > >
> > >
> > > On Mon, Apr 22, 2019 at 3:44 PM Chris Egerton 
> > wrote:
> > >
> > >> Hi Magesh,
> > >>
> > >> This is an exciting KIP! I have a few questions/comments but overall I
> > >> like
> > >> the direction it's headed in and hope to see it included in the
> Connect
> > >> framework soon.
> > >>
> > >> 1. With the proposed "consumer.", "producer.", and "admin." prefixes,
> > how
> > >> will this interact with connectors such as the upcoming Mirror Maker
> 2.0
> > >> (KIP-382) that already support properties with those prefixes? Would
> i

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi all,

I also have a draft implementation of the KIP
https://github.com/apache/kafka/pull/6624. I would still need to include
more tests and docs but I thought it would be useful to have this for the
KIP discussion. Looking forward to all of your valuable feedback.

Thanks
Magesh

On Tue, Apr 23, 2019 at 10:27 PM Magesh Nandakumar 
wrote:

> Chrise,
>
> Thanks a lot for your feedback. I will address them in order of your
> questions/comments.
>
> 1. Thanks for bringing this to my attention about KIP-382. I had a closer
> look at the KIP and IIUC, the KIP allows `consumer.` prefix for 
> SourceConnector
> and producer. prefix for SinkConnector since those are additional
> connector properties to help resolve the Kafka cluster other than the one
> Connect framework knows about. Whereas, the proposal in KIP-458 applies
> producer policies for SinkConnectors and consumer policies
> SourceConnectors.  So, from what I understand this new policy should work
> without any issues even for Mirror Maker 2.0.
> 2. I have updated the KIP to use a default value of null and use that to
> determine if we need to ignore overrides.
> 3. I would still prefer to keep the special 
> PrincipalConnectorClientConfigPolicy
> since that is one of the most common use cases one would choose to use this
> feature. If we make it a general case, that would involve users requiring
> to add additional configuration and they might require well more than just
> the list of configs but might also want some restriction on values. If the
> concern is about users wanting principal and also other configs, it would
> still be possible by means of a custom implementation. As is, I would
> prefer to keep the proposal to be the same for this. Let me know your
> thoughts.
>
> Thanks,
> Magesh
>
>
> On Mon, Apr 22, 2019 at 3:44 PM Chris Egerton  wrote:
>
>> Hi Magesh,
>>
>> This is an exciting KIP! I have a few questions/comments but overall I
>> like
>> the direction it's headed in and hope to see it included in the Connect
>> framework soon.
>>
>> 1. With the proposed "consumer.", "producer.", and "admin." prefixes, how
>> will this interact with connectors such as the upcoming Mirror Maker 2.0
>> (KIP-382) that already support properties with those prefixes? Would it be
>> possible for a user to configure MM2 with those properties without them
>> being interpreted as Connect client overrides, without isolating MM2 onto
>> its own cluster and using the IgnoreConnectorClientConfigPolicy policy?
>> 2. Is the IgnoreConnectorClientConfigPolicy class necessary? The default
>> for the connector.client.config.policy property could simply be null
>> instead of a new policy that, as far as I can tell, isn't an actual policy
>> in that its validate(...) method is never invoked and instead represents a
>> special case to the Connect framework that says "Drop all overrides and
>> never use me".
>> 3. The PrincipalConnectorClientConfigPolicy seems like a specific instance
>> of a more general use case: allow exactly a small set of overrides and no
>> others. Why not generalize here and create a policy that accepts a list of
>> allowed overrides during configuration?
>>
>> Thanks again for the KIP.
>>
>> Cheers,
>>
>> Chris
>>
>> On Fri, Apr 19, 2019 at 2:53 PM Magesh Nandakumar 
>> wrote:
>>
>> > Hi all,
>> >
>> > I've posted "KIP-458: Connector Client Config Override Policy", which
>> > allows users to override the connector client configurations based on a
>> > policy defined by the administrator.
>> >
>> > The KIP can be found at
>> >
>> >
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
>> > .
>> >
>> > Looking forward for the discussion on the KIP and all of your thoughts &
>> > feedback on this enhancement to Connect.
>> >
>> > Thanks,
>> > Magesh
>> >
>>
>

Re: [DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Chrise,

Thanks a lot for your feedback. I will address them in order of your
questions/comments.

1. Thanks for bringing this to my attention about KIP-382. I had a closer
look at the KIP and IIUC, the KIP allows `consumer.` prefix for SourceConnector
and producer. prefix for SinkConnector since those are additional connector
properties to help resolve the Kafka cluster other than the one Connect
framework knows about. Whereas, the proposal in KIP-458 applies producer
policies for SinkConnectors and consumer policies SourceConnectors.  So,
from what I understand this new policy should work without any issues even
for Mirror Maker 2.0.
2. I have updated the KIP to use a default value of null and use that to
determine if we need to ignore overrides.
3. I would still prefer to keep the special
PrincipalConnectorClientConfigPolicy
since that is one of the most common use cases one would choose to use this
feature. If we make it a general case, that would involve users requiring
to add additional configuration and they might require well more than just
the list of configs but might also want some restriction on values. If the
concern is about users wanting principal and also other configs, it would
still be possible by means of a custom implementation. As is, I would
prefer to keep the proposal to be the same for this. Let me know your
thoughts.

Thanks,
Magesh


On Mon, Apr 22, 2019 at 3:44 PM Chris Egerton  wrote:

> Hi Magesh,
>
> This is an exciting KIP! I have a few questions/comments but overall I like
> the direction it's headed in and hope to see it included in the Connect
> framework soon.
>
> 1. With the proposed "consumer.", "producer.", and "admin." prefixes, how
> will this interact with connectors such as the upcoming Mirror Maker 2.0
> (KIP-382) that already support properties with those prefixes? Would it be
> possible for a user to configure MM2 with those properties without them
> being interpreted as Connect client overrides, without isolating MM2 onto
> its own cluster and using the IgnoreConnectorClientConfigPolicy policy?
> 2. Is the IgnoreConnectorClientConfigPolicy class necessary? The default
> for the connector.client.config.policy property could simply be null
> instead of a new policy that, as far as I can tell, isn't an actual policy
> in that its validate(...) method is never invoked and instead represents a
> special case to the Connect framework that says "Drop all overrides and
> never use me".
> 3. The PrincipalConnectorClientConfigPolicy seems like a specific instance
> of a more general use case: allow exactly a small set of overrides and no
> others. Why not generalize here and create a policy that accepts a list of
> allowed overrides during configuration?
>
> Thanks again for the KIP.
>
> Cheers,
>
> Chris
>
> On Fri, Apr 19, 2019 at 2:53 PM Magesh Nandakumar 
> wrote:
>
> > Hi all,
> >
> > I've posted "KIP-458: Connector Client Config Override Policy", which
> > allows users to override the connector client configurations based on a
> > policy defined by the administrator.
> >
> > The KIP can be found at
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
> > .
> >
> > Looking forward for the discussion on the KIP and all of your thoughts &
> > feedback on this enhancement to Connect.
> >
> > Thanks,
> > Magesh
> >
>

Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

Chris,

I certainly would love to hear others thoughts on #1 but IMO, it might not
be as useful as ConnectorConfigs and as you mentioned, we could always add
it when the need arises.
Thanks for clarifying the details on my concern #2 regarding the
kafkaClusterId. While not a perfect fit in the interface, I'm not
completely opposed to having it in the interface. The other option, I can
think is to expose a connectClusterId() returning group.id + kafkaClusterId
(with some delimiter) rather than returning the kafkaClusterId. If we
choose to go this route, we can even make this a first-class citizen of the
Herder interface. Let me know what you think.

Thanks
Magesh

On Thu, Apr 18, 2019 at 2:45 PM Chris Egerton  wrote:

> Hi Magesh,
>
> Thanks for your comments. I'll address them in the order you provided them:
>
> 1 - Reason for exposing task configurations to REST extensions:
> Yes, the motivation is a little thin for exposing task configs to REST
> extensions. I can think of a few uses for this functionality, such as
> attempting to infer problematic configurations by examining failed tasks
> and comparing their configurations to the configurations of running tasks,
> but like you've indicated it's dubious that the best place for anything
> like that belongs in a REST extension.
> I'd be interested to hear others' thoughts, but right now I'm not too
> opposed to erring on the side of caution and leaving it out. Worst case, it
> takes another KIP to add this later on down the road, but that's a small
> price to pay to avoid adding support for a feature that nobody needs.
>
> 2. Usefulness of exposing Kafka cluster ID to REST extensions:
> As the KIP states, "the Kafka cluster ID may be useful for the purpose of
> uniquely identifying a Connect cluster from within a REST extension, since
> users may be running multiple Kafka clusters and the group.id for a
> distributed Connect cluster may not be sufficient to identify a cluster."
> Even though there may be producer or consumer overrides for
> bootstrap.servers present in the configuration for the worker, these will
> not affect which Kafka cluster is used as a backing store for connector
> configurations, offsets, and statuses, so the Kafka cluster ID for the
> worker in conjunction with the Connect group ID should be sufficient to
> uniquely identify a Connect cluster.
> We can and should document that the Connect cluster with overridden
> producer.bootstrap.servers or consumer.bootstrap.servers may be writing
> to/reading from a different Kafka cluster. However, REST extensions are
> already passed the configs for their worker through their configure(...)
> method, so they'd be able to detect any such overrides and act accordingly.
>
> Thanks again for your thoughts!
>
> Cheers,
>
> Chris
>
> On Thu, Apr 18, 2019 at 11:08 AM Magesh Nandakumar 
> wrote:
>
> > Hi Chris,
> >
> > Thanks for the KIP. Overall, it looks good and straightforward to me.
> >
> > I had a few questions on the new methods
> >
> > 1. I'm not sure if an extension would ever require the task configs. An
> > extension generally should only require the health and current state of
> the
> > connector which includes the connector config. I was wondering if there
> is
> > a specific reason it would need task configs.
> > 2. Also, I'm not convinced that kafkaClusterId() belongs to the
> > ConnectClusterState
> > interface. The interface is generally to provide information about the
> > Connect cluster and its information.  Also, the kafkaClusterId could
> > potentially change based on whether there is a "producer." or "consumer."
> > prefix, right?
> >
> > Having said that, I would prefer to have connectorConfigs which I think
> is
> > a great idea and addition to the interface. Let me know what you think.
> >
> > Thanks,
> > Magesh
> >
> > On Sat, Apr 13, 2019 at 9:00 PM Chris Egerton 
> wrote:
> >
> > > Hi all,
> > >
> > > I've posted "KIP-454: Expansion of the ConnectClusterState interface",
> > > which proposes that we add provide more information about the Connect
> > > cluster to REST extensions.
> > >
> > > The KIP can be found at
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-454%3A+Expansion+of+the+ConnectClusterState+interface
> > >
> > > I'm eager to hear people's thoughts on this and will appreciate any
> > > feedback.
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> >
>

[DISCUSS] KIP-458: Connector Client Config Override Policy

[Magesh Nandakumar]

Hi all,

I've posted "KIP-458: Connector Client Config Override Policy", which
allows users to override the connector client configurations based on a
policy defined by the administrator.

The KIP can be found at
https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy
.

Looking forward for the discussion on the KIP and all of your thoughts &
feedback on this enhancement to Connect.

Thanks,
Magesh

Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface

[Magesh Nandakumar]

Hi Chris,

Thanks for the KIP. Overall, it looks good and straightforward to me.

I had a few questions on the new methods

1. I'm not sure if an extension would ever require the task configs. An
extension generally should only require the health and current state of the
connector which includes the connector config. I was wondering if there is
a specific reason it would need task configs.
2. Also, I'm not convinced that kafkaClusterId() belongs to the
ConnectClusterState
interface. The interface is generally to provide information about the
Connect cluster and its information.  Also, the kafkaClusterId could
potentially change based on whether there is a "producer." or "consumer."
prefix, right?

Having said that, I would prefer to have connectorConfigs which I think is
a great idea and addition to the interface. Let me know what you think.

Thanks,
Magesh

On Sat, Apr 13, 2019 at 9:00 PM Chris Egerton  wrote:

> Hi all,
>
> I've posted "KIP-454: Expansion of the ConnectClusterState interface",
> which proposes that we add provide more information about the Connect
> cluster to REST extensions.
>
> The KIP can be found at
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-454%3A+Expansion+of+the+ConnectClusterState+interface
>
> I'm eager to hear people's thoughts on this and will appreciate any
> feedback.
>
> Cheers,
>
> Chris
>

Re: [VOTE] KIP-369 Alternative Partitioner to Support "Always Round-Robin" Selection

[Magesh Nandakumar]

+1 ( non-binding)

On Tue, Sep 4, 2018 at 3:39 AM M. Manna  wrote:

> Hello,
>
> I have made necessary changes as per the original discussion thread, and
> would like to put it for votes.
>
> Thank you very much for your suggestion and guidance so far.
>
> Regards,
>

Re: [DISCUSS] KIP-369 Alternative Partitioner to Support "Always Round-Robin" Selection

[Magesh Nandakumar]

+1 for this. The only small suggestion would be to possibly call this
RondRobinPartitioner which makes the intent obvious.

On Thu, Aug 30, 2018 at 5:31 PM Stephen Powis  wrote:

> Neat, this would be super helpful! I submitted this ages ago:
> https://issues.apache.org/jira/browse/KAFKA-
>
> On Fri, Aug 31, 2018 at 5:04 AM, Satish Duggana 
> wrote:
>
> > +including both dev and user mailing lists.
> >
> > Hi,
> > Thanks for the KIP.
> >
> > "* For us, the message keys represent some metadata which we use to
> either
> > ignore messages (if a loop-back to the sender), or log some
> information.*"
> >
> > Above statement was mentioned in the KIP about how key value is used. I
> > guess the topic is not configured to be compacted and you do not want to
> > have partitioning based on that key. IMHO, it qualifies more as a header
> > than a key. What do you think about building records with a specific
> header
> > and consumers to execute the logic whether to process or ignore the
> > messages based on that header value.
> >
> > Thanks,
> > Satish.
> >
> >
> > On Fri, Aug 31, 2018 at 1:32 AM, Satish Duggana <
> satish.dugg...@gmail.com>
> > wrote:
> >
> > > Hi,
> > > Thanks for the KIP.
> > >
> > > "* For us, the message keys represent some metadata which we use to
> > > either ignore messages (if a loop-back to the sender), or log some
> > > information.*"
> > >
> > > Above statement was mentioned in the KIP about how key value is used. I
> > > guess the topic is not configured to be compacted and you do not want
> to
> > > have partitioning based on that key. IMHO, it qualifies more as a
> header
> > > than a key. What do you think about building records with a specific
> > header
> > > and consumers to execute the logic whether to process or ignore the
> > > messages based on that header value.
> > >
> > > Thanks,
> > > Satish.
> > >
> > >
> > > On Fri, Aug 31, 2018 at 12:02 AM, M. Manna  wrote:
> > >
> > >> Hi Harsha,
> > >>
> > >> thanks for reading the KIP.
> > >>
> > >> The intent is to use the DefaultPartitioner logic for round-robin
> > >> selection
> > >> of partition regardless of key type.
> > >>
> > >> Implementing Partitioner interface isn’t the issue here, you would
> have
> > to
> > >> do that anyway if  you are implementing your own. But we also want
> this
> > to
> > >> be part of formal codebase.
> > >>
> > >> Regards,
> > >>
> > >> On Thu, 30 Aug 2018 at 16:58, Harsha  wrote:
> > >>
> > >> > Hi,
> > >> >   Thanks for the KIP. I am trying to understand the intent of
> the
> > >> > KIP.  Is the use case you specified can't be achieved by
> implementing
> > >> the
> > >> > Partitioner interface here?
> > >> > https://github.com/apache/kafka/blob/trunk/clients/src/main/
> > >> java/org/apache/kafka/clients/producer/Partitioner.java#L28
> > >> > .
> > >> > Use your custom partitioner to be configured in your producer
> clients.
> > >> >
> > >> > Thanks,
> > >> > Harsha
> > >> >
> > >> > On Thu, Aug 30, 2018, at 1:45 AM, M. Manna wrote:
> > >> > > Hello,
> > >> > >
> > >> > > I opened a very simple KIP and there exists a JIRA for it.
> > >> > >
> > >> > > I would be grateful if any comments are available for action.
> > >> > >
> > >> > > Regards,
> > >> >
> > >>
> > >
> > >
> >
>

Re: [DISCUSS] KIP-158: Kafka Connect should allow source connectors to set topic-specific settings for new topics

[Magesh Nandakumar]

Randall,

I originally thought that this proposal was a config only topic settings
and hence made the comment about configs being pass through. I just
realized that the connectors can also override and provide the
TopicSettings. With that in mind, I think the proposal looks great.
Looking forward to the feature.

Thanks,
Magesh

On Tue, Aug 28, 2018 at 8:53 PM Magesh Nandakumar 
wrote:

> I was wondering if it would be much simpler to just do a pass-through so
> that we can support any topic setting added in Kafka without any code
> changes in connect. Since these are for topics that will have the actual
> data stream, users might possibly need more flexibility in terms of how the
> topics get created.
>
> Thanks
> Magesh
>
> On Tue, Aug 28, 2018 at 4:56 PM Randall Hauch  wrote:
>
>> Do you think we should support name-value pairs, too?
>>
>> On Tue, Aug 28, 2018 at 6:41 PM Magesh Nandakumar 
>> wrote:
>>
>> > Randall,
>> >
>> > Thanks a lot for the KIP. I think this would be a great addition for
>> many
>> > source connectors.
>> > One clarification I had was regarding the topic settings that can be
>> > configured. Is it limited to the setting exposed in the TopicSettings
>> > interface?
>> >
>> > Thanks
>> > Magesh
>> >
>> > On Tue, Aug 21, 2018 at 7:59 PM Randall Hauch  wrote:
>> >
>> > > Okay, after much delay let's try this again for AK 2.1. Has anyone
>> found
>> > > any concerns? Stephane suggested that we allow updating topic
>> > > configurations (everything but partition count). I'm unconvinced that
>> > it's
>> > > worth the additional complexity in the implementation and the
>> > documentation
>> > > to explain the behavior. Changing several of the topic-specific
>> > > configurations have significant impact on broker behavior /
>> > functionality,
>> > > so IMO we need to proceed more cautiously.
>> > >
>> > > Stephane, do you have a particular use case in mind for updating topic
>> > > configurations on an existing topic?
>> > >
>> > > Randall
>> > >
>> > >
>> > > On Fri, Jan 26, 2018 at 4:20 PM Randall Hauch 
>> wrote:
>> > >
>> > > > The KIP deadline for 1.1 has already passed, but I'd like to restart
>> > this
>> > > > discussion so that we make the next release. I've not yet addressed
>> the
>> > > > previous comment about *existing* topics, but I'll try to do that
>> over
>> > > the
>> > > > next few weeks. Any other comments/suggestions/questions?
>> > > >
>> > > > Best regards,
>> > > >
>> > > > Randall
>> > > >
>> > > > On Thu, Oct 5, 2017 at 12:13 AM, Randall Hauch 
>> > wrote:
>> > > >
>> > > >> Oops. Yes, I meant “replication factor”.
>> > > >>
>> > > >> > On Oct 4, 2017, at 7:18 PM, Ted Yu  wrote:
>> > > >> >
>> > > >> > Randall:
>> > > >> > bq. AdminClient currently allows changing the replication
>> factory.
>> > > >> >
>> > > >> > By 'replication factory' did you mean 'replication factor' ?
>> > > >> >
>> > > >> > Cheers
>> > > >> >
>> > > >> >> On Wed, Oct 4, 2017 at 9:58 AM, Randall Hauch > >
>> > > >> wrote:
>> > > >> >>
>> > > >> >> Currently the KIP's scope is only topics that don't yet exist,
>> and
>> > we
>> > > >> have
>> > > >> >> to cognizant of race conditions between tasks with the same
>> > > connector.
>> > > >> I
>> > > >> >> think it is worthwhile to consider whether the KIP's scope
>> should
>> > > >> expand to
>> > > >> >> also address *existing* partitions, though it may not be
>> > appropriate
>> > > to
>> > > >> >> have as much control when changing the topic settings for an
>> > existing
>> > > >> >> topic. For example, changing the number of partitions (which the
>> > KIP
>> > > >> >> considers a "topic-specific setting" even though technically it
>> is
>> > > not)
>> > > >> >> shouldn't be done 

Re: [DISCUSS] KIP-158: Kafka Connect should allow source connectors to set topic-specific settings for new topics

[Magesh Nandakumar]

I was wondering if it would be much simpler to just do a pass-through so
that we can support any topic setting added in Kafka without any code
changes in connect. Since these are for topics that will have the actual
data stream, users might possibly need more flexibility in terms of how the
topics get created.

Thanks
Magesh

On Tue, Aug 28, 2018 at 4:56 PM Randall Hauch  wrote:

> Do you think we should support name-value pairs, too?
>
> On Tue, Aug 28, 2018 at 6:41 PM Magesh Nandakumar 
> wrote:
>
> > Randall,
> >
> > Thanks a lot for the KIP. I think this would be a great addition for many
> > source connectors.
> > One clarification I had was regarding the topic settings that can be
> > configured. Is it limited to the setting exposed in the TopicSettings
> > interface?
> >
> > Thanks
> > Magesh
> >
> > On Tue, Aug 21, 2018 at 7:59 PM Randall Hauch  wrote:
> >
> > > Okay, after much delay let's try this again for AK 2.1. Has anyone
> found
> > > any concerns? Stephane suggested that we allow updating topic
> > > configurations (everything but partition count). I'm unconvinced that
> > it's
> > > worth the additional complexity in the implementation and the
> > documentation
> > > to explain the behavior. Changing several of the topic-specific
> > > configurations have significant impact on broker behavior /
> > functionality,
> > > so IMO we need to proceed more cautiously.
> > >
> > > Stephane, do you have a particular use case in mind for updating topic
> > > configurations on an existing topic?
> > >
> > > Randall
> > >
> > >
> > > On Fri, Jan 26, 2018 at 4:20 PM Randall Hauch 
> wrote:
> > >
> > > > The KIP deadline for 1.1 has already passed, but I'd like to restart
> > this
> > > > discussion so that we make the next release. I've not yet addressed
> the
> > > > previous comment about *existing* topics, but I'll try to do that
> over
> > > the
> > > > next few weeks. Any other comments/suggestions/questions?
> > > >
> > > > Best regards,
> > > >
> > > > Randall
> > > >
> > > > On Thu, Oct 5, 2017 at 12:13 AM, Randall Hauch 
> > wrote:
> > > >
> > > >> Oops. Yes, I meant “replication factor”.
> > > >>
> > > >> > On Oct 4, 2017, at 7:18 PM, Ted Yu  wrote:
> > > >> >
> > > >> > Randall:
> > > >> > bq. AdminClient currently allows changing the replication factory.
> > > >> >
> > > >> > By 'replication factory' did you mean 'replication factor' ?
> > > >> >
> > > >> > Cheers
> > > >> >
> > > >> >> On Wed, Oct 4, 2017 at 9:58 AM, Randall Hauch 
> > > >> wrote:
> > > >> >>
> > > >> >> Currently the KIP's scope is only topics that don't yet exist,
> and
> > we
> > > >> have
> > > >> >> to cognizant of race conditions between tasks with the same
> > > connector.
> > > >> I
> > > >> >> think it is worthwhile to consider whether the KIP's scope should
> > > >> expand to
> > > >> >> also address *existing* partitions, though it may not be
> > appropriate
> > > to
> > > >> >> have as much control when changing the topic settings for an
> > existing
> > > >> >> topic. For example, changing the number of partitions (which the
> > KIP
> > > >> >> considers a "topic-specific setting" even though technically it
> is
> > > not)
> > > >> >> shouldn't be done blindly due to the partitioning impacts, and
> IIRC
> > > you
> > > >> >> can't reduce them (which we could verify before applying). Also,
> I
> > > >> don't
> > > >> >> think the AdminClient currently allows changing the replication
> > > >> factory. I
> > > >> >> think changing the topic configs is less problematic both from
> what
> > > >> makes
> > > >> >> sense for connectors to verify/change and from what the
> AdminClient
> > > >> >> supports.
> > > >> >>
> > > >> >> Even if we decide that it's not appropriate to change the
> settings
> > on
> > > >> an
> > > >> >> exi

Re: [DISCUSS] KIP-158: Kafka Connect should allow source connectors to set topic-specific settings for new topics

[Magesh Nandakumar]

Randall,

Thanks a lot for the KIP. I think this would be a great addition for many
source connectors.
One clarification I had was regarding the topic settings that can be
configured. Is it limited to the setting exposed in the TopicSettings
interface?

Thanks
Magesh

On Tue, Aug 21, 2018 at 7:59 PM Randall Hauch  wrote:

> Okay, after much delay let's try this again for AK 2.1. Has anyone found
> any concerns? Stephane suggested that we allow updating topic
> configurations (everything but partition count). I'm unconvinced that it's
> worth the additional complexity in the implementation and the documentation
> to explain the behavior. Changing several of the topic-specific
> configurations have significant impact on broker behavior / functionality,
> so IMO we need to proceed more cautiously.
>
> Stephane, do you have a particular use case in mind for updating topic
> configurations on an existing topic?
>
> Randall
>
>
> On Fri, Jan 26, 2018 at 4:20 PM Randall Hauch  wrote:
>
> > The KIP deadline for 1.1 has already passed, but I'd like to restart this
> > discussion so that we make the next release. I've not yet addressed the
> > previous comment about *existing* topics, but I'll try to do that over
> the
> > next few weeks. Any other comments/suggestions/questions?
> >
> > Best regards,
> >
> > Randall
> >
> > On Thu, Oct 5, 2017 at 12:13 AM, Randall Hauch  wrote:
> >
> >> Oops. Yes, I meant “replication factor”.
> >>
> >> > On Oct 4, 2017, at 7:18 PM, Ted Yu  wrote:
> >> >
> >> > Randall:
> >> > bq. AdminClient currently allows changing the replication factory.
> >> >
> >> > By 'replication factory' did you mean 'replication factor' ?
> >> >
> >> > Cheers
> >> >
> >> >> On Wed, Oct 4, 2017 at 9:58 AM, Randall Hauch 
> >> wrote:
> >> >>
> >> >> Currently the KIP's scope is only topics that don't yet exist, and we
> >> have
> >> >> to cognizant of race conditions between tasks with the same
> connector.
> >> I
> >> >> think it is worthwhile to consider whether the KIP's scope should
> >> expand to
> >> >> also address *existing* partitions, though it may not be appropriate
> to
> >> >> have as much control when changing the topic settings for an existing
> >> >> topic. For example, changing the number of partitions (which the KIP
> >> >> considers a "topic-specific setting" even though technically it is
> not)
> >> >> shouldn't be done blindly due to the partitioning impacts, and IIRC
> you
> >> >> can't reduce them (which we could verify before applying). Also, I
> >> don't
> >> >> think the AdminClient currently allows changing the replication
> >> factory. I
> >> >> think changing the topic configs is less problematic both from what
> >> makes
> >> >> sense for connectors to verify/change and from what the AdminClient
> >> >> supports.
> >> >>
> >> >> Even if we decide that it's not appropriate to change the settings on
> >> an
> >> >> existing topic, I do think it's advantageous to at least notify the
> >> >> connector (or task) prior to the first record sent to a given topic
> so
> >> that
> >> >> the connector can fail or issue a warning if it doesn't meet its
> >> >> requirements.
> >> >>
> >> >> Best regards,
> >> >>
> >> >> Randall
> >> >>
> >> >> On Wed, Oct 4, 2017 at 12:52 AM, Stephane Maarek <
> >> >> steph...@simplemachines.com.au> wrote:
> >> >>
> >> >>> Hi Randall,
> >> >>>
> >> >>> Thanks for the KIP. I like it
> >> >>> What happens when the target topic is already created but the
> configs
> >> do
> >> >>> not match?
> >> >>> i.e. wrong RF, num partitions, or missing / additional configs? Will
> >> you
> >> >>> attempt to apply the necessary changes or throw an error?
> >> >>>
> >> >>> Thanks!
> >> >>> Stephane
> >> >>>
> >> >>>
> >> >>> On 24/5/17, 5:59 am, "Mathieu Fenniak" <
> mathieu.fenn...@replicon.com
> >> >
> >> >>> wrote:
> >> >>>
> >> >>>Ah, yes, I see you a highlighted part that should've made this
> >> clear
> >> >>>to me the first read. :-)  Much clearer now!
> >> >>>
> >> >>>By the way, enjoyed your Debezium talk in NYC.
> >> >>>
> >> >>>Looking forward to this Kafka Connect change; it will allow me to
> >> >>>remove a post-deployment tool that I hacked together for the
> >> purpose
> >> >>>of ensuring auto-created topics have the right config.
> >> >>>
> >> >>>Mathieu
> >> >>>
> >> >>>
> >> >>>On Tue, May 23, 2017 at 11:38 AM, Randall Hauch <
> rha...@gmail.com>
> >> >>> wrote:
> >>  Thanks for the quick feedback, Mathieu. Yes, the first
> >> >> configuration
> >> >>> rule
> >>  whose regex matches will be applied, and no other rules will be
> >> >>> used. I've
> >>  updated the KIP to try to make this more clear, but let me know if
> >> >>> it's
> >>  still not clear.
> >> 
> >>  Best regards,
> >> 
> >>  Randall
> >> 
> >>  On Tue, May 23, 2017 at 10:07 AM, Mathieu Fenniak <
> >>  mathieu.fenn...@replicon.com> wrote:
> >> 
> >> > Hi Randall,
> >> >
> >> > Awesome, very much 

Re: [VOTE] KIP-305: Add Connect primitive number converters

[Magesh Nandakumar]

+1 (non-binding)

Thanks
Magesh

On Tue, May 22, 2018 at 4:23 PM, Randall Hauch  wrote:

> (bump so a few new subscribers see this thread.)
>
> On Tue, May 22, 2018 at 4:39 PM, Randall Hauch  wrote:
>
> > +1 (non-binding)
> >
> > On Tue, May 22, 2018 at 4:05 PM, Matthias J. Sax 
> > wrote:
> >
> >> +1 (binding)
> >>
> >> On 5/22/18 1:49 PM, Gwen Shapira wrote:
> >> > +1 (I can't believe we didn't have it until now...)
> >> >
> >> > On Tue, May 22, 2018 at 1:26 PM, Ewen Cheslack-Postava <
> >> e...@confluent.io>
> >> > wrote:
> >> >
> >> >> +1 (binding)
> >> >>
> >> >> -Ewen
> >> >>
> >> >> On Tue, May 22, 2018 at 9:29 AM Ted Yu  wrote:
> >> >>
> >> >>> +1
> >> >>>
> >> >>> On Tue, May 22, 2018 at 9:19 AM, Randall Hauch 
> >> wrote:
> >> >>>
> >>  I'd like to start a vote of a very straightforward proposal for
> >> Connect
> >> >>> to
> >>  add converters for the basic primitive number types: integer,
> short,
> >> >>> long,
> >>  double, and float that reuse Kafka's corresponding serdes. Here is
> >> the
> >> >>> KIP:
> >> 
> >>  https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>  305%3A+Add+Connect+primitive+number+converters
> >> 
> >> 
> >>  Best regards,
> >> 
> >>  Randall
> >> 
> >> >>>
> >> >>
> >> >
> >> >
> >> >
> >>
> >>
> >
>

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

[Magesh Nandakumar]

Thanks a lot Guozhang. I have updated the KIP to reflect that minor change.

On Mon, May 21, 2018 at 9:08 AM, Guozhang Wang <wangg...@gmail.com> wrote:

> Thanks for the KIP, +1 from me (binding)
>
> One minor suggestion: in "class TaskState" constructor, the passed in
> parameter of `worker` could be `workerId`, to be consistent with others in
> ConnectorState?
>
>
> Guozhang
>
>
> On Fri, May 18, 2018 at 9:44 AM, Randall Hauch <rha...@gmail.com> wrote:
>
> > +1 (non-binding)
> >
> > Regards,
> > Randall
> >
> > On Fri, May 18, 2018 at 11:32 AM, Konstantine Karantasis <
> > konstant...@confluent.io> wrote:
> >
> > > +1 (non-binding)
> > >
> > > - Konstantine
> > >
> > > On Thu, May 17, 2018 at 10:05 PM, Ewen Cheslack-Postava <
> > e...@confluent.io
> > > >
> > > wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > Thanks,
> > > > Ewen
> > > >
> > > > On Thu, May 17, 2018 at 12:16 PM Ted Yu <yuzhih...@gmail.com> wrote:
> > > >
> > > > > +1
> > > > >  Original message From: Gwen Shapira <
> > > g...@confluent.io>
> > > > > Date: 5/17/18  12:02 PM  (GMT-08:00) To: dev <dev@kafka.apache.org
> >
> > > > > Subject: Re: [VOTE] KIP-285: Connect Rest Extension Plugin
> > > > > LGTM. +1.
> > > > >
> > > > > On Wed, May 16, 2018 at 8:19 PM, Magesh Nandakumar <
> > > mage...@confluent.io
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Hello everyone,
> > > > > >
> > > > > > After a good round of discussions with excellent feedback and no
> > > major
> > > > > > objections, I would like to start a vote on KIP-285: Connect Rest
> > > > > Extension
> > > > > > Plugin.
> > > > > >
> > > > > > KIP: <
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > 285%3A+Connect+Rest+Extension+Plugin
> > > > > > >
> > > > > >
> > > > > >
> > > > > > JIRA: <*https://issues.apache.org/jira/browse/KAFKA-6776
> > > > > > <https://issues.apache.org/jira/browse/KAFKA-6776>*>
> > > > > >
> > > > > > Discussion thread: <
> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg86910.html>
> > > > > >
> > > > > > Thanks,
> > > > > > Magesh
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > *Gwen Shapira*
> > > > > Product Manager | Confluent
> > > > > 650.450.2760 | @gwenshap
> > > > > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> > > > > <http://www.confluent.io/blog>
> > > > >
> > > >
> > >
> >
>
>
>
> --
> -- Guozhang
>

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

[Magesh Nandakumar]

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 Liveliness probe
for kubernetes. Depending on how critical the connector is one could decide
the liveliness based on various task status. This is something very
specific to SLA requirements for the connector for a user. Hence exposing
this to Extension does allow users to implement healthchecks based on their
requirements. Let me know your thoughts.

Thanks
Magesh

On Fri, May 18, 2018 at 4:05 PM, Jason Gustafson <ja...@confluent.io> wrote:

> 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 motivation for
> why a plugin would need this. Do the current status endpoints not integrate
> well with common alerting systems? Is that something that can be fixed?
>
> Thanks,
> Jason
>
> On Thu, May 17, 2018 at 10:05 PM, Ewen Cheslack-Postava <e...@confluent.io
> >
> wrote:
>
> > 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
> > > >>

Re: [VOTE] KIP-298: Error Handling in Connect kafka

[Magesh Nandakumar]

+1 (non-binding)

Thanks,
Magesh

On Fri, May 18, 2018 at 2:38 PM, Konstantine Karantasis <
konstant...@confluent.io> wrote:

> +1 (non-binding)
>
> - Konstantine
>
> On Thu, May 17, 2018 at 12:58 AM, Arjun Satish 
> wrote:
>
> > All,
> >
> > Many thanks for all the feedback on KIP-298. Highly appreciate the time
> and
> > effort you all put into it.
> >
> > I've updated the KIP accordingly, and would like to start to start a vote
> > on it.
> >
> > KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 298%3A+Error+Handling+in+Connect
> > JIRA: https://issues.apache.org/jira/browse/KAFKA-6738
> > Discussion Thread: https://www.mail-archive.com/
> > dev@kafka.apache.org/msg87660.html
> >
> > Thanks very much!
> >
>

Re: [DISCUSS] KIP-298: Error Handling in Connect

[Magesh Nandakumar]

Arjun,

Thanks for all the updates. I think it looks great and I don't have any
other concerns.

Thanks
Magesh

On Fri, May 18, 2018 at 2:36 PM, Konstantine Karantasis <
konstant...@confluent.io> wrote:

> The updated version of the KIP that uses the dead-letter-queue only for
> sink records and only to store the raw record data looks better and easier
> to understand.
> I think it's moving to the right direction.
>
> No further comments from my side.
>
> Thanks Arjun!
>
> - Konstantine
>
> On Fri, May 18, 2018 at 1:07 AM, Arjun Satish 
> wrote:
>
> > Ewen,
> >
> > Thanks a lot for your comments!
> >
> > 1. For errors.retry.delay.max.ms, yes we plan to use exponential
> backoffs
> > with an fixed initial value. Updated the KIP to say this.
> >
> > 2. A failed operation will be retried (potentially multiple times). If
> all
> > the retries fail, we declare this to be an error. This is where tolerance
> > kicks in. Hence, you can have 0 retries, but infinite tolerance (
> > errors.tolerance.limit = -1), where we will never retry any failure, but
> > all of bad records will be skipped. Updated the KIP. Hopefully this is
> > clear now.
> >
> > 3. Yes, for error messages we have some base information (what operation
> > failed and with what exception and stacktrace, for example). Hence, the
> > three configs. The main reason for having properties for disabling
> messages
> > and configs is to avoid logging sensitive information to unsecured
> > locations (for example, the file logs). Updated the KIP to describe this.
> >
> > I think topic name should be mandatory: if we have a default topic, then
> > all the connectors in a cluster will produce messages into it, making it
> > confusing to read from. We could have a default pattern for constructing
> > topic names, for example: a format like ${connector-name}-errors.
> >
> > 4. The reason for multiple clusters is to allow users with sensitive data
> > to log errors into secure clusters. There are defaults for these
> > properties, but if you think this is making the config too complex, we
> can
> > drop the errors.deadletterqueue.producer.* properties from this
> > implementation.
> >
> > 5. I had mentioned that the format is in JSON in the proposed changes
> > section. Updated the public interface section to say this again. We could
> > provide overrides for the Converter used here, and use an AvroConverter
> > instead, which should preserve the structure and schema of the data. The
> > avro binary would be base64 encoded in the logged records. But yes, this
> > brings in configurable converters and their configurations which
> introduces
> > a new level of complexity (AvroConverters and their dependency on Schema
> > Registry, for instance). Hence, they were not included in this proposal.
> >
> > Another option is to add a StructSerializer and StructDeserializer, which
> > can retain the schema and structure of the Structs in the schema. If we
> do
> > this, non-Java clients which need to read these error records would need
> to
> > port the deserialization logic. Ultimately, we need to indicate what the
> > record looks like, and
> >
> > Could you point out what is unclear w.r.t reprocessing?
> >
> > Let me know what you think.
> >
> >
> > On Thu, May 17, 2018 at 11:02 PM, Ewen Cheslack-Postava <
> e...@confluent.io
> > >
> > wrote:
> >
> > > A few more thoughts -- might not change things enough to affect a vote,
> > but
> > > still some things to consider:
> > >
> > > * errors.retry.delay.max.ms -- this defines the max, but I'm not
> seeing
> > > where we define the actual behavior. Is this intentional, or should we
> > just
> > > say that it is something like exponential, based on a starting delay
> > value?
> > > * I'm not sure I understand tolerance vs retries? They sound generally
> > the
> > > same -- tolerance sounds like # of retries since it is defined in terms
> > of
> > > failures.
> > > * errors.log.enable -- it's unclear why this shouldn't just be
> > > errors.log.include.configs
> > > || errors.log.include.messages (and include clauses for any other
> flags).
> > > If there's just some base info, that's fine, but the explanation of the
> > > config should make that clear.
> > > * errors.deadletterqueue.enable - similar question here about just
> > enabling
> > > based on other relevant configs. seems like additional config
> complexity
> > > for users when the topic name is absolutely going to be a basic
> > requirement
> > > anyway.
> > > * more generally related to dlq, it seems we're trying to support
> > multiple
> > > clusters here -- is there a reason for this? it's not that costly, but
> > one
> > > thing supporting this requires is an entirely separate set of configs,
> > > ACLs, etc. in contrast, assuming an additional topic on the same
> cluster
> > > we're already working with keeps things quite simple. do we think this
> > > complexity is worth it? elsewhere, we've seen the complexity of
> multiple
> > > 

Re: [VOTE] KIP-297: Externalizing Secrets for Connect Configurations

[Magesh Nandakumar]

Thanks Robert, this looks great

+1 (non-binding)

On Thu, May 17, 2018 at 5:35 PM, Colin McCabe  wrote:

> Thanks, Robert!
>
> +1 (non-binding)
>
> Colin
>
>
> On Thu, May 17, 2018, at 14:15, Robert Yokota wrote:
> > Hi Colin,
> >
> > I've changed the KIP to have a composite object returned from get().
> It's
> > probably the most straightforward option.  Please let me know if you have
> > any other concerns.
> >
> > Thanks,
> > Robert
> >
> > On Thu, May 17, 2018 at 11:44 AM, Robert Yokota 
> wrote:
> >
> > >
> > >
> > > Hi Colin,
> > >
> > > My last response was not that clear, so let me back up and explain a
> bit
> > > more.
> > >
> > > Some secret managers, such as Vault (and maybe Keywhiz) have the
> notion of
> > > a lease duration or a TTL for a path.  Every path can have a different
> > > TTL.  This is period after which the value of the keys at the given
> path
> > > may be invalid.  It can be used to indicate a rotation will be done.
> In
> > > the cause of the Vault integration with AWS, Vault will actually
> delete the
> > > secrets from AWS at the moment the TTL expires.  A TTL could be used by
> > > other ConfigProviders, such as a FileConfigProvider, to indicate that
> all
> > > the secrets at a given path (file), will be rotated on a regular basis.
> > >
> > > I would like to expose the TTL in the APIs somewhere.  The TTL can be
> made
> > > available at the time get() is called.  Connect already has a built in
> > > ScheduledExecutor, so Connect can just use the TTL to schedule a
> Connector
> > > restart.  Originally, I had exposed the TTL in a ConfigContext
> interface
> > > passed to the get() method.  To reduce the number of APIs, I placed it
> on
> > > the onChange() method.  This means at the time of get(), onChange()
> would
> > > be called with a TTL.  The Connector's implementation of the callback
> would
> > > use onChange() with the TTL to schedule a restart.
> > >
> > > If you think this is overloading onChange() too much, I could add the
> > > ConfigContext back to get():
> > >
> > >
> > > Map get(ConfigContext ctx, String path);
> > >
> > > public interface ConfigContext {
> > >
> > > void willExpire(String path, long ttl);
> > >
> > > }
> > >
> > >
> > >
> > > or I could separate out the TTL method in the callback:
> > >
> > >
> > > public interface ConfigChangeCallback {
> > >
> > > void willExpire(String path, long ttl);
> > >
> > > void onChange(String path, Map values);
> > > }
> > >
> > >
> > >
> > > Or we could return a composite object from get():
> > >
> > > ConfigData get(String path);
> > >
> > > public class ConfigData {
> > >
> > >   Map data;
> > >   long ttl;
> > >
> > > }
> > >
> > >
> > > Do you have a preference Colin?
> > >
> > > Thanks,
> > > Robert
> > >
> > >
> > > On Thu, May 17, 2018 at 9:27 AM, Colin McCabe 
> wrote:
> > >
> > >> Hi Robert,
> > >>
> > >> Hmm.  I thought that if you're using ConfigChangeCallback, you are
> > >> relying on the ConfigProvider to make a callback to you when the
> > >> configuration has changed.  So isn't that always the "push model"
> (where
> > >> the ConfigProvider pushes changes to Connect).  If you want the "pull
> > >> model" where you initiate updates, you can simply call
> ConfigProvider#get
> > >> directly, right?
> > >>
> > >> The actual implementation of ConfigProvider subclasses will depend on
> the
> > >> type of configuration storage mechanism on the backend.  In the case
> of
> > >> Vault, it sounds like we need to have something like a
> ScheduledExecutor
> > >> which re-fetches keys after a certain amount of time.
> > >>
> > >> As an aside, what does a "lease duration" mean for a configuration
> key?
> > >> Does that mean Vault will reject changes to the configuration key if
> I try
> > >> to make them within the lease duration?  Or is this like a period
> after
> > >> which a password is automatically rotated?
> > >>
> > >> On Wed, May 16, 2018, at 22:25, Robert Yokota wrote:
> > >> > Hi Colin,
> > >> >
> > >> > > With regard to delayMs, can’t we just restart the
> > >> > > Connector when the keys are actually changed?
> > >> >
> > >> > Currently the VaultConfigProvider does not find out when values for
> keys
> > >> > have changed.  You could do this with a poll model (with a
> > >> > background thread in the ConfigProvider), but since for each
> key-value
> > >> > pair, Vault provides a lease duration stating exactly when a value
> for a
> > >> > key will change in the future, this is an alternative model of just
> > >> passing
> > >> > the lease duration to the client (in this case the Connector), to
> allow
> > >> it
> > >> > to determine what to do (such as schedule a restart).   This may
> allow
> > >> one
> > >> > to avoid the complexity of figuring out a proper poll interval (with
> > >> lease
> > >> > durations of varying periods), or worrying about putting too much
> load
> 

Re: [DISCUSS] KIP-305: Add Connect primitive number converters

[Magesh Nandakumar]

Thanks Randall for the KIP. I think it will be super useful and looks
pretty straightforward to me.

Thanks
Magesh

On Thu, May 17, 2018 at 4:15 PM, Randall Hauch  wrote:

> I'd like to start discussion of a very straightforward proposal for Connect
> to add converters for the basic primitive number types: integer, short,
> long, double, and float. Here is the KIP:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 305%3A+Add+Connect+primitive+number+converters
>
> As mentioned in the KIP, I've created a pull request (
> https://github.com/apache/kafka/pull/5034) for those looking for
> implementation details.
>
> Any feedback is appreciated.
>
> Best regards,
>
> Randall
>

[VOTE] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar]

Hello everyone,

After a good round of discussions with excellent feedback and no major
objections, I would like to start a vote on KIP-285: Connect Rest Extension
Plugin.

KIP: <
https://cwiki.apache.org/confluence/display/KAFKA/KIP-285%3A+Connect+Rest+Extension+Plugin
>


JIRA: <*https://issues.apache.org/jira/browse/KAFKA-6776
*>

Discussion thread: <
https://www.mail-archive.com/dev@kafka.apache.org/msg86910.html>

Thanks,
Magesh

Re: [DISCUSS] KIP-298: Error Handling in Connect

[Magesh Nandakumar]

s. Thank you!
> > > >
> > > > 4. I'll add an example log4j config to show we can take logs from a
> > class
> > > > and redirect it to a different location. Made a note in the PR for
> > this.
> > > >
> > > > 5. When we talk about logging messages, this could mean instances of
> > > > SinkRecords or SourceRecords. When we disable logging of messages,
> > these
> > > > records would be replaced by a "null". If you think it makes sense,
> > > instead
> > > > of completely dropping the object, we could drop only the key and
> value
> > > > objects from ConnectRecord? That way some context will still be
> > retained.
> > > >
> > > > 6. Yes, for now I think it is good to have explicit config in
> > Connectors
> > > > which dictates the error handling behavior. If this becomes an
> > > > inconvenience, we can think of having a cluster global default, or
> > better
> > > > defaults in the configs.
> > > >
> > > > Best,
> > > >
> > > >
> > > > On Tue, May 15, 2018 at 1:07 PM, Magesh Nandakumar <
> > mage...@confluent.io
> > > >
> > > > wrote:
> > > >
> > > >> Hi Arjun,
> > > >>
> > > >> I think this a great KIP and would be a great addition to have in
> > > connect.
> > > >> Had a couple of minor questions:
> > > >>
> > > >> 1. What would be the value in logging the connector config using
> > > >> errors.log.include.configs
> > > >> for every message?
> > > >> 2. Not being picky on format here but it might be clearer if the
> > > behavior
> > > >> is called out for each stage separately and what the connector
> > > developers
> > > >> need to do ( may be a tabular format). Also, I think all retriable
> > > >> exception when talking to Broker are never propagated to the Connect
> > > >> Framework since the producer is configured to try indefinitely
> > > >> 3. If a message fails in serialization, would the raw bytes be
> > available
> > > >> to
> > > >> the dlq or the error log
> > > >> 4. Its not necessary to mention in KIP, but it might be better to
> > > separate
> > > >> the error records to a separate log file as part of the default
> log4j
> > > >> properties
> > > >> 5. If we disable message logging, would there be any other metadata
> > > >> available like offset that helps reference the record?
> > > >> 6. If I need error handler for all my connectors, would I have to
> set
> > it
> > > >> up
> > > >> for each of them? I would think most people might want the behavior
> > > >> applied
> > > >> to all the connectors.
> > > >>
> > > >> Let me know your thoughts :).
> > > >>
> > > >> Thanks
> > > >> Magesh
> > > >>
> > > >> On Tue, May 8, 2018 at 11:59 PM, Arjun Satish <
> arjun.sat...@gmail.com
> > >
> > > >> wrote:
> > > >>
> > > >> > All,
> > > >> >
> > > >> > I'd like to start a discussion on adding ways to handle and report
> > > >> record
> > > >> > processing errors in Connect. Please find a KIP here:
> > > >> >
> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >> > 298%3A+Error+Handling+in+Connect
> > > >> >
> > > >> > Any feedback will be highly appreciated.
> > > >> >
> > > >> > Thanks very much,
> > > >> > Arjun
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

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

[Magesh Nandakumar]

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

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

[Magesh Nandakumar]

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*

AbstractState
ConnectClusterState
ConnectorHealth
ConnectorState
ConnectorType
TaskState

Runtime would include the following implementations

*org.apache.kafka.connect.runtime.rest*

ConnectRestExtensionContextImpl
ConnectRestConfigurable

*org.apache.kafka.connect.runtime.health*

ConnectClusterStateImpl

Let me know your thoughts.

Thanks
Magesh

On Wed, May 16, 2018 at 3:50 PM, Ewen Cheslack-Postava <e...@confluent.io>
wrote:

> 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 name to just be
> under o.a.k.connect.rest or something like that would better keep this
> distinction clear and would also help shorten it a bit -- the packages are
> getting quite deeply nested with some of the new naming.
> * The cluster state classes probably shouldn't be under a rest package.
> That's where we're exposing them for public APIs currently, but it's not
> really specific to REST stuff in any way. I think we should house those
> somewhere more generic so they won't be awkward to reuse if we decided to
> (e.g. you could imagine extensions that provide this directly for metrics.
> * Currently we have the State classes nested inside ConnectorHealth class.
> I think this makes those classes more annoying to use. Is there a reason
> for them to be nested or can we just pull them out to the same level as
> ConnectorHealth?
>
> -Ewen
>
> On Tue, May 15, 2018 at 9: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 

Re: [DISCUSS] KIP-298: Error Handling in Connect

[Magesh Nandakumar]

Hi Arjun,

I think this a great KIP and would be a great addition to have in connect.
Had a couple of minor questions:

1. What would be the value in logging the connector config using
errors.log.include.configs
for every message?
2. Not being picky on format here but it might be clearer if the behavior
is called out for each stage separately and what the connector developers
need to do ( may be a tabular format). Also, I think all retriable
exception when talking to Broker are never propagated to the Connect
Framework since the producer is configured to try indefinitely
3. If a message fails in serialization, would the raw bytes be available to
the dlq or the error log
4. Its not necessary to mention in KIP, but it might be better to separate
the error records to a separate log file as part of the default log4j
properties
5. If we disable message logging, would there be any other metadata
available like offset that helps reference the record?
6. If I need error handler for all my connectors, would I have to set it up
for each of them? I would think most people might want the behavior applied
to all the connectors.

Let me know your thoughts :).

Thanks
Magesh

On Tue, May 8, 2018 at 11:59 PM, Arjun Satish 
wrote:

> All,
>
> I'd like to start a discussion on adding ways to handle and report record
> processing errors in Connect. Please find a KIP here:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 298%3A+Error+Handling+in+Connect
>
> Any feedback will be highly appreciated.
>
> Thanks very much,
> Arjun
>

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

[Magesh Nandakumar]

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

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

[Magesh Nandakumar]

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

Re: [DISCUSS] KIP-297: Externalizing Secrets for Connect Configurations

[Magesh Nandakumar]

Hi Robert,

Thanks for the KIP. I think, this will be a great addition to the
framework. I think, will be great if the KIP can elaborate a little bit
more on how implementations would look like with an example.
Also, would be good to provide a reference implementation as well.

The other questions I had were

1.  How would the framework get the delayMs for void scheduleConfigReload(
long delayMs);
2. Would the start methods in SourceTask and SinkTask get the configs with
all the indirect references resolved. If so, trying to understand
the intent of the config() in SourceTaskContext and the SinkTaskContext
3. What if the provider itself needs some kind of secrets to be configured
to connect to it? I assume that's out of scope for this proposal but wanted
to clarify it.

Thanks
Magesh

On Tue, May 8, 2018 at 1:52 PM, Robert Yokota  wrote:

> Hi,
>
> I would like to start a discussion for KIP-297 to externalize secrets from
> Kafka Connect configurations.  Any feedback is appreciated.
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 297%3A+Externalizing+Secrets+for+Connect+Configurations
> >
>
> JIRA: 
>
> Thanks in advance,
> Robert
>

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

[Magesh Nandakumar]

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

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

[Magesh Nandakumar]

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 API module?
   2. All of the rest entities today live in  runtime. We are
   exposing org.apache.kafka.connect.runtime.rest.entities.ConnectorStateInfo
   via our plugin interface. So , was wondering if these entities can move to
   API since they are already in a way public.


If we don't want to do that all the new interfaces would need to live in
the runtime. This might affect some minor details in the KIP. Let me know
your thoughts.

Thanks
Magesh


On Thu, Apr 19, 2018 at 10:17 PM, 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:

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

[Magesh Nandakumar]

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/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 &qu

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

[Magesh Nandakumar]

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

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

[Magesh Nandakumar]

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

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.

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

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

[Magesh Nandakumar]

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 <g...@confluent.io> wrote:

> 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 implementation and it doesn't work, it will be harder to know if
> it is me or Kafka...
>
> On Wed, Apr 11, 2018 at 9:47 AM, Magesh Nandakumar <mage...@confluent.io>
> wrote:
>
> > 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 Wed, Apr 11, 2018 at 9:42 AM, Gwen Shapira <g...@confluent.io> wrote:
> >
> > > 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 interfaces, we always contributed a simple
> > > implementation that can be used for testing, experimentation and as an
> > > example to future implementers. The JSON converter is one such example.
> > Or
> > > the ZK authorizer.
> > >
> > > Do you plan on including a simple implementation as part of this KIP as
> > > well?
> > >
> > > Gwen
> > >
> > > On Tue, Apr 10, 2018 at 10:14 PM, 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
> > > >
> > >
> > >
> > >
> > > --
> > > *Gwen Shapira*
> > > Product Manager | Confluent
> > > 650.450.2760 | @gwenshap
> > > Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> > > <http://www.confluent.io/blog>
> > >
> >
>
>
>
> --
> *Gwen Shapira*
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> <http://www.confluent.io/blog>
>

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

[Magesh Nandakumar]

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 Wed, Apr 11, 2018 at 9:42 AM, Gwen Shapira <g...@confluent.io> wrote:

> 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 interfaces, we always contributed a simple
> implementation that can be used for testing, experimentation and as an
> example to future implementers. The JSON converter is one such example. Or
> the ZK authorizer.
>
> Do you plan on including a simple implementation as part of this KIP as
> well?
>
> Gwen
>
> On Tue, Apr 10, 2018 at 10:14 PM, 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
> >
>
>
>
> --
> *Gwen Shapira*
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog
> <http://www.confluent.io/blog>
>

[DISCUSS] KIP-285: Connect Rest Extension Plugin

[Magesh Nandakumar]

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

[Magesh Nandakumar]

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