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