You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Chris Egerton <ch...@confluent.io> on 2019/05/08 00:30:32 UTC
Re: [DISCUSS] KIP-454: Expansion of the ConnectClusterState interface
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 <ch...@confluent.io> 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 <
> konstantine@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 <ma...@confluent.io>
>> wrote:
>>
>> > Thanks a lot, Chris. The KIP looks good to me.
>> >
>> > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <ch...@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 <
>> mageshn@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 <ch...@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 <
>> > > mageshn@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 <
>> chrise@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
>> > > > > > 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 <
>> > > > > mageshn@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 <
>> > > chrise@confluent.io
>> > > > >
>> > > > > > > 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 <
>> > > > > > > mageshn@confluent.io
>> > > > > > > > >
>> > > > > > > > > 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 <
>> > > > > chrise@confluent.io
>> > > > > > >
>> > > > > > > > > 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
Posted by Rajini Sivaram <ra...@gmail.com>.
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 <ch...@confluent.io> 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/<name>/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/<name> and
> /connectors/<name>/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 <ra...@gmail.com>
> 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 <ch...@confluent.io>
> 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 <ch...@confluent.io>
> > > 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 <
> > > > konstantine@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 <
> > > mageshn@confluent.io>
> > > >> wrote:
> > > >>
> > > >> > Thanks a lot, Chris. The KIP looks good to me.
> > > >> >
> > > >> > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <
> chrise@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 <
> > > >> mageshn@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 <
> > > chrise@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 <
> > > >> > > mageshn@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 <
> > > >> chrise@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
> > > >> > > > > > 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 <
> > > >> > > > > mageshn@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 <
> > > >> > > chrise@confluent.io
> > > >> > > > >
> > > >> > > > > > > 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 <
> > > >> > > > > > > mageshn@confluent.io
> > > >> > > > > > > > >
> > > >> > > > > > > > > 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 <
> > > >> > > > > chrise@confluent.io
> > > >> > > > > > >
> > > >> > > > > > > > > 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
Posted by Chris Egerton <ch...@confluent.io>.
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/<name>/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/<name> and
/connectors/<name>/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 <ra...@gmail.com>
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 <ch...@confluent.io> 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 <ch...@confluent.io>
> > 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 <
> > > konstantine@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 <
> > mageshn@confluent.io>
> > >> wrote:
> > >>
> > >> > Thanks a lot, Chris. The KIP looks good to me.
> > >> >
> > >> > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <ch...@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 <
> > >> mageshn@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 <
> > chrise@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 <
> > >> > > mageshn@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 <
> > >> chrise@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
> > >> > > > > > 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 <
> > >> > > > > mageshn@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 <
> > >> > > chrise@confluent.io
> > >> > > > >
> > >> > > > > > > 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 <
> > >> > > > > > > mageshn@confluent.io
> > >> > > > > > > > >
> > >> > > > > > > > > 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 <
> > >> > > > > chrise@confluent.io
> > >> > > > > > >
> > >> > > > > > > > > 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
Posted by Rajini Sivaram <ra...@gmail.com>.
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 <ch...@confluent.io> 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 <ch...@confluent.io>
> 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 <
> > konstantine@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 <
> mageshn@confluent.io>
> >> wrote:
> >>
> >> > Thanks a lot, Chris. The KIP looks good to me.
> >> >
> >> > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <ch...@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 <
> >> mageshn@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 <
> chrise@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 <
> >> > > mageshn@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 <
> >> chrise@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
> >> > > > > > 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 <
> >> > > > > mageshn@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 <
> >> > > chrise@confluent.io
> >> > > > >
> >> > > > > > > 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 <
> >> > > > > > > mageshn@confluent.io
> >> > > > > > > > >
> >> > > > > > > > > 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 <
> >> > > > > chrise@confluent.io
> >> > > > > > >
> >> > > > > > > > > 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
> >> > > > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > >
> >> > > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>