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

[Rajini Sivaram]

Hi Chris,

Thanks for the explanation. Since there are workarounds and we are not
making it any worse, this should be fine.

Regards,

Rajini


On Wed, May 8, 2019 at 8:06 PM Chris Egerton  wrote:

> Hi Rajini,
>
> That was an initial concern of mine as well but I think we should be fine.
> Connect REST extensions are already capable of intercepting requests that
> contain new connector configurations, through POST calls to the /connectors
> endpoint and PUT calls to /connectors//config. The additional method
> you pointed out would extend that capability to include not just new
> connector configurations but existing connector configurations (by querying
> the Connect herder) as well.
>
> Neither should be a problem because, as of the merging of
> https://github.com/apache/kafka/pull/6129 (which addressed
> https://issues.apache.org/jira/browse/KAFKA-5117), both of those
> configurations can make use of the ConfigProvider mechanism in Connect to
> hide sensitive configs.
>
> If that mechanism is not used, connector configurations are available via
> the Connect REST API through GET calls to /connectors/ and
> /connectors//config, so it seems reasonable to enable REST extensions
> to view them as well.
>
> I hope this addresses your concerns; I'm happy to continue the discussion
> if any follow-up is necessary.
>
> Thanks for your thoughts!
>
> Cheers,
>
> Chris
>
> On Wed, May 8, 2019 at 11:19 AM Rajini Sivaram 
> wrote:
>
> > Hi Chris,
> >
> > Thanks for the KIP, looks good. I have just one question. Can `
> > ConnectClusterState#connectorConfig()` return any sensitive configs like
> > passwords?
> >
> > Thanks,
> >
> > Rajini
> >
> > On Wed, May 8, 2019 at 1:30 AM Chris Egerton 
> wrote:
> >
> > > Hi all,
> > >
> > > Now that  KAFKA-8304 (https://issues.apache.org/jira/browse/KAFKA-8304
> ),
> > > which was a blocker, has been addressed, I've published a PR for these
> > > changes: https://github.com/apache/kafka/pull/6584
> > >
> > > Thanks to everyone who's voted so far! If anyone else is interested,
> the
> > > voting thread can be found here:
> > > https://www.mail-archive.com/dev@kafka.apache.org/msg97458.html.
> Current
> > > status: +1 binding, +2 non-binding.
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Tue, Apr 30, 2019 at 12:40 PM Chris Egerton 
> > > wrote:
> > >
> > > > Hi Konstantine,
> > > >
> > > > I've updated the KIP to add default method implementations to the
> list
> > of
> > > > rejected alternatives. Technically this makes the changes in the KIP
> > > > backwards incompatible, but I think I agree that for the majority of
> > > cases
> > > > where it would even be an issue a compile-time error is likely to be
> > more
> > > > beneficial than one at run time.
> > > >
> > > > Thanks for your thoughts and thanks for the LGTM!
> > > >
> > > > Cheers,
> > > >
> > > > Chris
> > > >
> > > > On Mon, Apr 29, 2019 at 12:29 PM Konstantine Karantasis <
> > > > konstant...@confluent.io> wrote:
> > > >
> > > >> Hi Chris,
> > > >>
> > > >> Thanks for considering my suggestion regarding default
> implementations
> > > for
> > > >> the new methods.
> > > >> However, given that these implementations don't seem to have sane
> > > defaults
> > > >> and throw UnsupportedOperationException, I think we'll be better
> > without
> > > >> defaults.
> > > >> Seems that a compile time error is preferable here, for those who
> want
> > > to
> > > >> upgrade their implementations.
> > > >>
> > > >> Otherwise, the KIP LGTM.
> > > >>
> > > >> Konstantine
> > > >>
> > > >> On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar <
> > > mage...@confluent.io>
> > > >> wrote:
> > > >>
> > > >> > Thanks a lot, Chris. The KIP looks good to me.
> > > >> >
> > > >> > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <
> chr...@confluent.io>
> > > >> 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 <
> > > >> mage...@confluent.io>
> > > >> > > 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 <
> > > chr...@confluent.io>
> > > >> > > wrote:
> > > >> > > >
> > > >> > > > > Hi Magesh,
> > > >> > > > >
> > > >> > > > > Expanding the type we use to convey cluster metadata from
> > just a
> > > >> > Kafka
> > > >> > > > 

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

[Chris Egerton]

Hi Rajini,

That was an initial concern of mine as well but I think we should be fine.
Connect REST extensions are already capable of intercepting requests that
contain new connector configurations, through POST calls to the /connectors
endpoint and PUT calls to /connectors//config. The additional method
you pointed out would extend that capability to include not just new
connector configurations but existing connector configurations (by querying
the Connect herder) as well.

Neither should be a problem because, as of the merging of
https://github.com/apache/kafka/pull/6129 (which addressed
https://issues.apache.org/jira/browse/KAFKA-5117), both of those
configurations can make use of the ConfigProvider mechanism in Connect to
hide sensitive configs.

If that mechanism is not used, connector configurations are available via
the Connect REST API through GET calls to /connectors/ and
/connectors//config, so it seems reasonable to enable REST extensions
to view them as well.

I hope this addresses your concerns; I'm happy to continue the discussion
if any follow-up is necessary.

Thanks for your thoughts!

Cheers,

Chris

On Wed, May 8, 2019 at 11:19 AM Rajini Sivaram 
wrote:

> Hi Chris,
>
> Thanks for the KIP, looks good. I have just one question. Can `
> ConnectClusterState#connectorConfig()` return any sensitive configs like
> passwords?
>
> Thanks,
>
> Rajini
>
> On Wed, May 8, 2019 at 1:30 AM Chris Egerton  wrote:
>
> > Hi all,
> >
> > Now that  KAFKA-8304 (https://issues.apache.org/jira/browse/KAFKA-8304),
> > which was a blocker, has been addressed, I've published a PR for these
> > changes: https://github.com/apache/kafka/pull/6584
> >
> > Thanks to everyone who's voted so far! If anyone else is interested, the
> > voting thread can be found here:
> > https://www.mail-archive.com/dev@kafka.apache.org/msg97458.html. Current
> > status: +1 binding, +2 non-binding.
> >
> > Cheers,
> >
> > Chris
> >
> > On Tue, Apr 30, 2019 at 12:40 PM Chris Egerton 
> > wrote:
> >
> > > Hi Konstantine,
> > >
> > > I've updated the KIP to add default method implementations to the list
> of
> > > rejected alternatives. Technically this makes the changes in the KIP
> > > backwards incompatible, but I think I agree that for the majority of
> > cases
> > > where it would even be an issue a compile-time error is likely to be
> more
> > > beneficial than one at run time.
> > >
> > > Thanks for your thoughts and thanks for the LGTM!
> > >
> > > Cheers,
> > >
> > > Chris
> > >
> > > On Mon, Apr 29, 2019 at 12:29 PM Konstantine Karantasis <
> > > konstant...@confluent.io> wrote:
> > >
> > >> Hi Chris,
> > >>
> > >> Thanks for considering my suggestion regarding default implementations
> > for
> > >> the new methods.
> > >> However, given that these implementations don't seem to have sane
> > defaults
> > >> and throw UnsupportedOperationException, I think we'll be better
> without
> > >> defaults.
> > >> Seems that a compile time error is preferable here, for those who want
> > to
> > >> upgrade their implementations.
> > >>
> > >> Otherwise, the KIP LGTM.
> > >>
> > >> Konstantine
> > >>
> > >> On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar <
> > mage...@confluent.io>
> > >> wrote:
> > >>
> > >> > 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 <
> > >> mage...@confluent.io>
> > >> > > 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 <
> > chr...@confluent.io>
> > >> > > 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
> 

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

[Rajini Sivaram]

Hi Chris,

Thanks for the KIP, looks good. I have just one question. Can `
ConnectClusterState#connectorConfig()` return any sensitive configs like
passwords?

Thanks,

Rajini

On Wed, May 8, 2019 at 1:30 AM Chris Egerton  wrote:

> Hi all,
>
> Now that  KAFKA-8304 (https://issues.apache.org/jira/browse/KAFKA-8304),
> which was a blocker, has been addressed, I've published a PR for these
> changes: https://github.com/apache/kafka/pull/6584
>
> Thanks to everyone who's voted so far! If anyone else is interested, the
> voting thread can be found here:
> https://www.mail-archive.com/dev@kafka.apache.org/msg97458.html. Current
> status: +1 binding, +2 non-binding.
>
> Cheers,
>
> Chris
>
> On Tue, Apr 30, 2019 at 12:40 PM Chris Egerton 
> wrote:
>
> > Hi Konstantine,
> >
> > I've updated the KIP to add default method implementations to the list of
> > rejected alternatives. Technically this makes the changes in the KIP
> > backwards incompatible, but I think I agree that for the majority of
> cases
> > where it would even be an issue a compile-time error is likely to be more
> > beneficial than one at run time.
> >
> > Thanks for your thoughts and thanks for the LGTM!
> >
> > Cheers,
> >
> > Chris
> >
> > On Mon, Apr 29, 2019 at 12:29 PM Konstantine Karantasis <
> > konstant...@confluent.io> wrote:
> >
> >> Hi Chris,
> >>
> >> Thanks for considering my suggestion regarding default implementations
> for
> >> the new methods.
> >> However, given that these implementations don't seem to have sane
> defaults
> >> and throw UnsupportedOperationException, I think we'll be better without
> >> defaults.
> >> Seems that a compile time error is preferable here, for those who want
> to
> >> upgrade their implementations.
> >>
> >> Otherwise, the KIP LGTM.
> >>
> >> Konstantine
> >>
> >> On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar <
> mage...@confluent.io>
> >> wrote:
> >>
> >> > 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 <
> >> mage...@confluent.io>
> >> > > 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 <
> chr...@confluent.io>
> >> > > 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?
> >> > > > > >
> >> > 

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

[Chris Egerton]

Hi all,

Now that  KAFKA-8304 (https://issues.apache.org/jira/browse/KAFKA-8304),
which was a blocker, has been addressed, I've published a PR for these
changes: https://github.com/apache/kafka/pull/6584

Thanks to everyone who's voted so far! If anyone else is interested, the
voting thread can be found here:
https://www.mail-archive.com/dev@kafka.apache.org/msg97458.html. Current
status: +1 binding, +2 non-binding.

Cheers,

Chris

On Tue, Apr 30, 2019 at 12:40 PM Chris Egerton  wrote:

> Hi Konstantine,
>
> I've updated the KIP to add default method implementations to the list of
> rejected alternatives. Technically this makes the changes in the KIP
> backwards incompatible, but I think I agree that for the majority of cases
> where it would even be an issue a compile-time error is likely to be more
> beneficial than one at run time.
>
> Thanks for your thoughts and thanks for the LGTM!
>
> Cheers,
>
> Chris
>
> On Mon, Apr 29, 2019 at 12:29 PM Konstantine Karantasis <
> konstant...@confluent.io> wrote:
>
>> Hi Chris,
>>
>> Thanks for considering my suggestion regarding default implementations for
>> the new methods.
>> However, given that these implementations don't seem to have sane defaults
>> and throw UnsupportedOperationException, I think we'll be better without
>> defaults.
>> Seems that a compile time error is preferable here, for those who want to
>> upgrade their implementations.
>>
>> Otherwise, the KIP LGTM.
>>
>> Konstantine
>>
>> On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar 
>> wrote:
>>
>> > 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 <
>> mage...@confluent.io>
>> > > 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 <
>> chr...@confluent.io
>> > >
>> > > > > 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 

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

[Chris Egerton]

Hi Konstantine,

I've updated the KIP to add default method implementations to the list of
rejected alternatives. Technically this makes the changes in the KIP
backwards incompatible, but I think I agree that for the majority of cases
where it would even be an issue a compile-time error is likely to be more
beneficial than one at run time.

Thanks for your thoughts and thanks for the LGTM!

Cheers,

Chris

On Mon, Apr 29, 2019 at 12:29 PM Konstantine Karantasis <
konstant...@confluent.io> wrote:

> Hi Chris,
>
> Thanks for considering my suggestion regarding default implementations for
> the new methods.
> However, given that these implementations don't seem to have sane defaults
> and throw UnsupportedOperationException, I think we'll be better without
> defaults.
> Seems that a compile time error is preferable here, for those who want to
> upgrade their implementations.
>
> Otherwise, the KIP LGTM.
>
> Konstantine
>
> On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar 
> wrote:
>
> > 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 <
> mage...@confluent.io>
> > > 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 <
> chr...@confluent.io
> > >
> > > > > 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
> > 

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

[Konstantine Karantasis]

Hi Chris,

Thanks for considering my suggestion regarding default implementations for
the new methods.
However, given that these implementations don't seem to have sane defaults
and throw UnsupportedOperationException, I think we'll be better without
defaults.
Seems that a compile time error is preferable here, for those who want to
upgrade their implementations.

Otherwise, the KIP LGTM.

Konstantine

On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar 
wrote:

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

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

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

[Chris Egerton]

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 
> > 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 group.id +
> > > > > kafkaClusterId
> > > > > (with some delimiter) rather than returning the kafkaClusterId. 

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

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

[Chris Egerton]

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

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

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

[Chris Egerton]

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

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

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

[Chris Egerton]

Hi Konstantine,

I didn't fully grasp the rationale behind adding default implementations
for the new interface at first; thanks for clarifying. I agree, this would
be good to include. I've updated the KIP with proposed defaults for the new
methods that simply throw UnsupportedOperationExceptions, and noted this
default behavior in the proposed Javadoc for the interface.

Thanks again for your thoughts!

Cheers,

Chris


On Tue, Apr 23, 2019 at 10:06 AM Konstantine Karantasis <
konstant...@confluent.io> wrote:

> Thanks for the additions to the KIP Chris! The descriptions are quite clear
> now.
>
> With respect to 2) my main point was to say that you are targeting a
> version that assumes Java 8, so adding the additional methods as default is
> an option.
> I think it's good practice to do that, even if we think it's unlikely there
> are any implementations of an interface outside apache/kafka repo. WDYT?
>
> Konstantine
>
> On Fri, Apr 19, 2019 at 2:26 PM Chris Egerton  wrote:
>
> > Hi Konstantine,
> >
> > Thanks for your comments. I'll respond to them in the order you provided
> > them:
> >
> > 1. Clarify of ConnectClusterState code block:
> > Good point. I've altered the code block in the KIP to remove the outer
> > 'interface ConnectClusterState {' snippet and include only the additional
> > methods. Happy to make further changes if there's still chance for
> > confusion here.
> >
> > 2. Targeted version:
> > I think it makes sense to target the 2.3 release for this KIP (time
> > permitting). The changes to the code base would consist of an additional
> > feature and not a bug fix, so I'm doubtful this qualifies to be
> backported.
> >
> > 3. ConnectorTaskId vs Integer:
> > I tried to follow the convention of the ConnectorHealth class, which
> > exposes task states as a map that uses task IDs (represented as Integer
> > objects) for keys, instead of ConnectTaskId. There was also some
> discussion
> > on KIP-285 about relying on classes in the Connect runtime module (as
> > opposed to the api module) and how that should be avoided, so if we
> wanted
> > to use a more sophisticated key for the return type of the taskConfigs
> > method, we'd probably need to implement our own for use in the Connect
> api
> > module. I'm not opposed to this, but there didn't seem to be good enough
> > reason to propose it initially given the convention set by the
> > ConnectorHealth class.
> >
> > 4. Config retrieval from herder:
> > I've added a paragraph to the KIP that gives some details on how
> > configurations will be retrieved from the herder; it's basically the same
> > as with the current ConnectClusterStateImpl class but with a few method
> > calls changed here and there.
> >
> > Thanks again for your thoughts!
> >
> > Cheers,
> >
> > Chris
> >
> > On Fri, Apr 19, 2019 at 11:24 AM Konstantine Karantasis <
> > konstant...@confluent.io> wrote:
> >
> > > Resending an email I sent this Monday but didn't make it to the mailing
> > > list:
> > >
> > > 
> > >
> > > Hi Chris,
> > >
> > > thanks for the KIP!
> > > I have the following initial comments:
> > >
> > > 1. In its current form, the code snippet gives the impression that this
> > is
> > > a new interface or an interface that completely replaces the existing
> > one.
> > > It's not clear that the interface is extended. You think we could
> > represent
> > > this extension in a better way? (I'm aware that in the text you say
> that
> > > these methods are additional, but the code block gives a partial view
> of
> > > this interface).
> > >
> > > 2. In the compatibility page it'd be nice to read which version this
> > > feature is targeting. Now, given that KIP-285 was introduced in 2.0.0,
> I
> > > wonder if it'd make sense to have default methods for the new interface
> > > methods that you suggest adding.
> > >
> > > 3. Any reason why ConnectorTaskId is not used instead of Integer as key
> > > type in taskConfigs? This class is already part of the connect-api
> > package
> > > and I'd imagine reusing it might allow for fewer transformations
> between
> > > task config maps currently used and the new ones that will be returned
> by
> > > this interface method.
> > >
> > > 4. Finally, there's a mention on the interface javadoc about how these
> > > configs are retrieved using the Connect herder, but it's not clear
> > whether
> > > the leader of the workers' group will be queried or not for this
> > > information. I think a paragraph describing what are the assumptions
> with
> > > respect to what consists a "current" task configuration and how this is
> > > retrieved would be valuable here.
> > >
> > > Best,
> > > Konstantine
> > >
> > >
> > >
> > > 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 

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

[Konstantine Karantasis]

Thanks for the additions to the KIP Chris! The descriptions are quite clear
now.

With respect to 2) my main point was to say that you are targeting a
version that assumes Java 8, so adding the additional methods as default is
an option.
I think it's good practice to do that, even if we think it's unlikely there
are any implementations of an interface outside apache/kafka repo. WDYT?

Konstantine

On Fri, Apr 19, 2019 at 2:26 PM Chris Egerton  wrote:

> Hi Konstantine,
>
> Thanks for your comments. I'll respond to them in the order you provided
> them:
>
> 1. Clarify of ConnectClusterState code block:
> Good point. I've altered the code block in the KIP to remove the outer
> 'interface ConnectClusterState {' snippet and include only the additional
> methods. Happy to make further changes if there's still chance for
> confusion here.
>
> 2. Targeted version:
> I think it makes sense to target the 2.3 release for this KIP (time
> permitting). The changes to the code base would consist of an additional
> feature and not a bug fix, so I'm doubtful this qualifies to be backported.
>
> 3. ConnectorTaskId vs Integer:
> I tried to follow the convention of the ConnectorHealth class, which
> exposes task states as a map that uses task IDs (represented as Integer
> objects) for keys, instead of ConnectTaskId. There was also some discussion
> on KIP-285 about relying on classes in the Connect runtime module (as
> opposed to the api module) and how that should be avoided, so if we wanted
> to use a more sophisticated key for the return type of the taskConfigs
> method, we'd probably need to implement our own for use in the Connect api
> module. I'm not opposed to this, but there didn't seem to be good enough
> reason to propose it initially given the convention set by the
> ConnectorHealth class.
>
> 4. Config retrieval from herder:
> I've added a paragraph to the KIP that gives some details on how
> configurations will be retrieved from the herder; it's basically the same
> as with the current ConnectClusterStateImpl class but with a few method
> calls changed here and there.
>
> Thanks again for your thoughts!
>
> Cheers,
>
> Chris
>
> On Fri, Apr 19, 2019 at 11:24 AM Konstantine Karantasis <
> konstant...@confluent.io> wrote:
>
> > Resending an email I sent this Monday but didn't make it to the mailing
> > list:
> >
> > 
> >
> > Hi Chris,
> >
> > thanks for the KIP!
> > I have the following initial comments:
> >
> > 1. In its current form, the code snippet gives the impression that this
> is
> > a new interface or an interface that completely replaces the existing
> one.
> > It's not clear that the interface is extended. You think we could
> represent
> > this extension in a better way? (I'm aware that in the text you say that
> > these methods are additional, but the code block gives a partial view of
> > this interface).
> >
> > 2. In the compatibility page it'd be nice to read which version this
> > feature is targeting. Now, given that KIP-285 was introduced in 2.0.0, I
> > wonder if it'd make sense to have default methods for the new interface
> > methods that you suggest adding.
> >
> > 3. Any reason why ConnectorTaskId is not used instead of Integer as key
> > type in taskConfigs? This class is already part of the connect-api
> package
> > and I'd imagine reusing it might allow for fewer transformations between
> > task config maps currently used and the new ones that will be returned by
> > this interface method.
> >
> > 4. Finally, there's a mention on the interface javadoc about how these
> > configs are retrieved using the Connect herder, but it's not clear
> whether
> > the leader of the workers' group will be queried or not for this
> > information. I think a paragraph describing what are the assumptions with
> > respect to what consists a "current" task configuration and how this is
> > retrieved would be valuable here.
> >
> > Best,
> > Konstantine
> >
> >
> >
> > 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 

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

[Chris Egerton]

Hi Konstantine,

Thanks for your comments. I'll respond to them in the order you provided
them:

1. Clarify of ConnectClusterState code block:
Good point. I've altered the code block in the KIP to remove the outer
'interface ConnectClusterState {' snippet and include only the additional
methods. Happy to make further changes if there's still chance for
confusion here.

2. Targeted version:
I think it makes sense to target the 2.3 release for this KIP (time
permitting). The changes to the code base would consist of an additional
feature and not a bug fix, so I'm doubtful this qualifies to be backported.

3. ConnectorTaskId vs Integer:
I tried to follow the convention of the ConnectorHealth class, which
exposes task states as a map that uses task IDs (represented as Integer
objects) for keys, instead of ConnectTaskId. There was also some discussion
on KIP-285 about relying on classes in the Connect runtime module (as
opposed to the api module) and how that should be avoided, so if we wanted
to use a more sophisticated key for the return type of the taskConfigs
method, we'd probably need to implement our own for use in the Connect api
module. I'm not opposed to this, but there didn't seem to be good enough
reason to propose it initially given the convention set by the
ConnectorHealth class.

4. Config retrieval from herder:
I've added a paragraph to the KIP that gives some details on how
configurations will be retrieved from the herder; it's basically the same
as with the current ConnectClusterStateImpl class but with a few method
calls changed here and there.

Thanks again for your thoughts!

Cheers,

Chris

On Fri, Apr 19, 2019 at 11:24 AM Konstantine Karantasis <
konstant...@confluent.io> wrote:

> Resending an email I sent this Monday but didn't make it to the mailing
> list:
>
> 
>
> Hi Chris,
>
> thanks for the KIP!
> I have the following initial comments:
>
> 1. In its current form, the code snippet gives the impression that this is
> a new interface or an interface that completely replaces the existing one.
> It's not clear that the interface is extended. You think we could represent
> this extension in a better way? (I'm aware that in the text you say that
> these methods are additional, but the code block gives a partial view of
> this interface).
>
> 2. In the compatibility page it'd be nice to read which version this
> feature is targeting. Now, given that KIP-285 was introduced in 2.0.0, I
> wonder if it'd make sense to have default methods for the new interface
> methods that you suggest adding.
>
> 3. Any reason why ConnectorTaskId is not used instead of Integer as key
> type in taskConfigs? This class is already part of the connect-api package
> and I'd imagine reusing it might allow for fewer transformations between
> task config maps currently used and the new ones that will be returned by
> this interface method.
>
> 4. Finally, there's a mention on the interface javadoc about how these
> configs are retrieved using the Connect herder, but it's not clear whether
> the leader of the workers' group will be queried or not for this
> information. I think a paragraph describing what are the assumptions with
> respect to what consists a "current" task configuration and how this is
> retrieved would be valuable here.
>
> Best,
> Konstantine
>
>
>
> 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 

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

[Konstantine Karantasis]

Resending an email I sent this Monday but didn't make it to the mailing
list:



Hi Chris,

thanks for the KIP!
I have the following initial comments:

1. In its current form, the code snippet gives the impression that this is
a new interface or an interface that completely replaces the existing one.
It's not clear that the interface is extended. You think we could represent
this extension in a better way? (I'm aware that in the text you say that
these methods are additional, but the code block gives a partial view of
this interface).

2. In the compatibility page it'd be nice to read which version this
feature is targeting. Now, given that KIP-285 was introduced in 2.0.0, I
wonder if it'd make sense to have default methods for the new interface
methods that you suggest adding.

3. Any reason why ConnectorTaskId is not used instead of Integer as key
type in taskConfigs? This class is already part of the connect-api package
and I'd imagine reusing it might allow for fewer transformations between
task config maps currently used and the new ones that will be returned by
this interface method.

4. Finally, there's a mention on the interface javadoc about how these
configs are retrieved using the Connect herder, but it's not clear whether
the leader of the workers' group will be queried or not for this
information. I think a paragraph describing what are the assumptions with
respect to what consists a "current" task configuration and how this is
retrieved would be valuable here.

Best,
Konstantine



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

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

[Chris Egerton]

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

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
>

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

[Chris Egerton]

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