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 <chr...@confluent.io> 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 <chr...@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 <
> > mage...@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 <chr...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>
