Hi Chris, Thanks for considering my suggestion regarding default implementations for the new methods. However, given that these implementations don't seem to have sane defaults and throw UnsupportedOperationException, I think we'll be better without defaults. Seems that a compile time error is preferable here, for those who want to upgrade their implementations.
Otherwise, the KIP LGTM. Konstantine On Thu, Apr 25, 2019 at 10:29 PM Magesh Nandakumar <mage...@confluent.io> wrote: > Thanks a lot, Chris. The KIP looks good to me. > > On Thu, Apr 25, 2019 at 7:35 PM Chris Egerton <chr...@confluent.io> wrote: > > > Hi Magesh, > > > > Sounds good; I've updated the KIP to make ConnectClusterDetails an > > interface. If we want to leave the door open to expand it in the future > it > > definitely makes sense to treat it similarly to how we're treating the > > ConnectClusterState interface now. > > > > Thanks, > > > > Chris > > > > On Thu, Apr 25, 2019 at 4:18 PM Magesh Nandakumar <mage...@confluent.io> > > wrote: > > > > > HI Chrise, > > > > > > Overall it looks good to me. Just one last comment - I was wondering if > > > ConnectClusterDetail should be an interface just like > > ConnectClusterState. > > > > > > Thanks, > > > Magesh > > > > > > On Thu, Apr 25, 2019 at 3:54 PM Chris Egerton <chr...@confluent.io> > > wrote: > > > > > > > Hi Magesh, > > > > > > > > Expanding the type we use to convey cluster metadata from just a > Kafka > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
