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