Hi Greg, I'm confused about the behavior for GET /connector-plugins?connectorsOnly=false. Won't it still only expose the latest version for each plugin, instead of the range of versions available?
I'm hoping we can provide a flow where people need at most two REST calls to discover 1) the complete set of plugins available on the worker (which is already possible with the endpoint under discussion) and 2) the set of versions available for a specific plugin on the worker (which doesn't appear to be possible, at least for some plugin types). This wouldn't require any out-of-band knowledge and would be valuable for connector users (who may want to, for example, know what their options are when considering a plugin upgrade) and cluster administrators (who could use it as a sanity check for the setup of their Kafka Connect clusters without having to pore over log files). As far as modifying the content of the GET /connector-plugins/<plugin>/config endpoint to include a "version" property goes, I think your point about returning that property from requests to that endpoint that include a version query parameter is salient, but it also unfortunately applies to all types of plugin. I don't think it should completely disqualify that option. I also wasn't imagining adding anything besides that single property to that endpoint, so no new "predicate" property for SMTs, no new "negate" property for predicates, and no new "type" property for either. I'm not necessarily opposed to adding those, but it can be done without being pulled into the scope of this KIP. So to summarize how I imagine people might use the REST API after this KIP: 1. Discover the set of plugins on the worker via GET /connector-plugins?connectorsOnly=false 2. For any of those plugins, discover the set of available versions by examining the recommended values for the "version" property after hitting the GET GET /connector-plugins/<plugin>/config endpoint I don't think this is necessarily optimal since plucking the "version" property out of the response from that endpoint might be a PITA for ad hoc queries via, e.g., curl, but I think it'd be enough for this KIP. Still open to other options though, and if I've misunderstood anything in the proposal, please let me know! Cheers, Chris On Wed, May 22, 2024 at 4:05 PM Greg Harris <greg.har...@aiven.io.invalid> wrote: > Hey Chris, > > Thanks for your comments, and I'm glad that it seems like we're > aligning on the vision here. > > > An > > alternative could be to change existing behavior to fail fast on any > > invalid default converter configuration instead of just for invalid > > versions > > I suppose if this is landing in 4.0, we have the opportunity to break > compatibility and strictly validate the worker class configs, and > could have a new consistent behavior instead of inconsistent but > backwards-compatible behavior. > > > no other part of the KIP requires this change. > > This is correct, and a compelling argument. I'm fine leaving the > strict worker validation off of this configuration to potentially be > added later. If a KIP was raised to perform strict validation of the > worker config, it would include the version config, and can address > backwards compatibility for both configs together. > > > RE exposing the version property in the > /connector-plugins/<plugin>/config > > endpoint, the behavior is inconsistent across plugin types. > > Yes it is, and that started in a bugfix, once people started using > this endpoint: https://issues.apache.org/jira/browse/KAFKA-14843 . I > wasn't planning on converging the behavior in this KIP, as I > considered it out-of-scope, and was going to follow the current > behavior. To summarize: > > GET /connector-plugins/<connector>/config will emit: > * connector.class (already implemented) > * connector.version (new) > * key.converter (already implemented) > * key.converter.version (new) > * value.converter (already implemented) > * value.converter.version (new) > * header.converter (already implemented) > * header.converter.version (new) > But will NOT emit: > * transforms.<alias>.type > * transforms.<alias>.version > * transforms.<alias>.predicate > * predicates.<alias>.type > * predicates.<alias>.version > * predicates.<alias>.negate > > GET /connector-plugins/<converter>/config will NOT emit: > * "" (there's not even a ".class" prefix!) > * version > > GET /connector-plugins/<transform>/config will NOT emit: > * type > * version > * predicate > > GET /connector-plugins/<predicate>/config will NOT emit: > * type > * version > * negate > > Do you want the converter, transform, and predicate endpoints changed? > Do you want just "version", or do you want all of the prefixed configs > including "type", "predicate" and "negate"? How would you want to > handle the converters? > And when I say the configs are "not part of the plugin config itself" > I mean that saying that GET > /connector-plugins/Flatten$Key/config?version=3.8.0 has a "version" > config that must be "3.8.0" is a little bit nonsense, as the version > is already specified. > > > IMO > > it's worth including this information somewhere directly accessible > without > > having to provide a full connector config. FWIW I'd be fine with GET > > /connector-plugins/<plugin>/versions as a first-class endpoint > > You don't have to provide a configuration to call GET > /connector-plugins?connectorsOnly=false , is that endpoint not close > enough to what you have in mind? See also the Rejected Alternative > "Adding new REST API endpoints" > > If you're calling /connector-plugins/<transform>/config, you know the > name of a plugin right? That either comes from out-of-band knowledge, > validating a connector config, or calling GET > /connector-plugins?connectorsOnly=false. > * If you have out-of-band knowledge of plugin classes, perhaps you > have out-of-band knowledge of versions too. > * If you've just validated a connector config, there should be an > accompanying "version" field there with an accurate default value and > recommenders. > * If you've called GET /connector-plugins?connectorsOnly=false, that > endpoint includes version information. > > Thanks, > Greg > > On Wed, May 22, 2024 at 11:05 AM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > > Hi Greg, > > > > Hope you had a nice weekend! Gonna try to keep things concise. > > > > Concluded points: > > > > RE version recommenders, I agree it's likely that programmatic UIs will > > already be able to handle dynamic configuration definitions, and the > detail > > about SMTs is a great point. I still anticipate some awkwardness with > > connector versions, though: if the latest version supports some new > > properties, then a user switches to an earlier version, a UI may respond > by > > wiping values for these properties. I guess we can bite the bullet, > though. > > > > RE double-dinging during preflight validation for invalid versions, I > like > > the analogy with login credentials. I'm convinced that the proposal in > the > > KIP is best 👍 > > > > Continued points: > > > > RE failing on worker startup, sorry, I should be clearer: there is no > _new_ > > justification for it that doesn't also apply to existing behavior. We > > shouldn't diverge from existing behavior solely for this new case. An > > alternative could be to change existing behavior to fail fast on any > > invalid default converter configuration instead of just for invalid > > versions, but I'd vote to just stick to existing behavior and not > > complicate things, especially since no other part of the KIP requires > this > > change. > > > > RE exposing the version property in the > /connector-plugins/<plugin>/config > > endpoint, the behavior is inconsistent across plugin types. Hitting the > > endpoint for the FileStreamSinkConnector on version 3.7.0 yields a > response > > that includes, among other things, the "topics", "topics.regex", and > > "errors.tolerance" properties. I see that we don't do this everywhere > (the > > examples you cite for SMT and converter properties are accurate), but IMO > > it's worth including this information somewhere directly accessible > without > > having to provide a full connector config. FWIW I'd be fine with GET > > /connector-plugins/<plugin>/versions as a first-class endpoint either > > instead of or in addition to adding recommended values for all plugin > > versions. > > > > Thanks for your continued work on this KIP, and with the progress we're > > making I'm optimistic about its chances of appearing in 4.0.0. > > > > Cheers, > > > > Chris > > > > On Wed, May 15, 2024 at 1:22 PM Greg Harris <greg.har...@aiven.io.invalid > > > > wrote: > > > > > Hey Chris, > > > > > > Thanks for your quick follow up. > > > > > > > But this risk is already present with > > > > existing error cases, and I don't see anything that justifies > changing > > > > existing behavior with an invalid converter class, or diverging from > it > > > in > > > > the case of invalid converter versions. > > > > > > The justification is to fail-fast, and prevent REST API users from > > > receiving errors from bad configs that they didn't write, or maybe > > > don't even know apply to them. > > > Up until recently errors in these configurations surfaced as failures > > > to create the connector, or failures to start, and you made them > > > fail-fast during validation. I think this change is in the same > > > spirit, pulling the validation further forward and not letting errors > > > lie dormant. > > > > > > And to call back to your original concern about interrupting > > > connectors that explicitly provide these configurations and don't use > > > the worker configs: I expect that operators with a majority of these > > > sorts of clients aren't going to be setting the worker .version > > > properties, because it would have no effect on the majority of their > > > connectors. They would be able to rely on backwards-compatibility and > > > continue to ignore the class properties. > > > > > > > CLI > > > > and programmatic UI developers will want to develop their own tooling > > > > layers. > > > > > > This is a very compelling argument. Snehashis do you want to figure > > > out a REST API design for this use-case? > > > > > > > Regarding the GET /connector-plugins/<plugin>/config endpoint, I was > > > > thinking about the response for non-connector plugins, e.g., > > > > GET /connector-plugins/RegexRouter/config. Would a "version" property > > > > appear with recommended values? > > > > > > I intended for the ".version" property to be like other framework > > > configs (".class", ".type", ".predicate", ".negate") where they are > > > inside the plugin namespace, but not part of the plugin config itself. > > > Perhaps we can deviate from those configs because it would aid in > > > discovering other valid `GET > > > /connector-plugins/<plugin>/config?version=` calls, without calling > > > `GET /connector-plugins?connectorsOnly=false`. > > > I don't really feel strongly either way. > > > > > > > if a user changes the > > > > connector version in, e.g., a dropdown menu, then the UI either has > to > > > > re-fetch the ConfigDef for the new version, or risk operating on > stale > > > > information > > > > > > This is a really interesting situation, thanks for finding that! This > > > is already a footgun with transformations and predicates; Once you > > > fill out a transformation class (which includes a recommender) you > > > will then see all of the transformations configurations. > > > I think this means that UI developers should have already developed > > > the infrastructure for handling dynamic recommenders, or else they've > > > had this bug since KIP-66 in 2017. It may require some manual > > > attention to roll-out support for the ".version" properties > > > specifically, but I don't think that should prevent us from providing > > > this information in a natural place. > > > > > > > but will it be possible to configure > > > > a connector to use two instances of the same transform or predicate, > but > > > > with different versions for each? > > > > > > Yes, and this is included in the example in the KIP, > > > `transforms.flatten-latest` and `transforms.flatten-old`. This will > > > work in the natural way, with the two instances having a different > > > version if configured for that. If a user wants to pin the same > > > version of a plugin for every instance, they will need to provide the > > > version config multiple times and keep them in-sync themselves. > > > > > > > I would have expected the error > > > > to only be attributed to the version property, and for the class > property > > > > to be reported as valid. > > > > > > I considered this, and then went back and changed it. To quote someone > > > else, this is to catch "misspelling cat as dog". For example a user > > > types DogConverter and meant to type CatConverter, and then types a > > > version which is valid for CatConverter, conceptually the error is in > > > the class name and not the version. > > > That's a very contrived scenario, but I think similar arguments are > > > used for attributing validation errors to endpoints/urls/hostnames > > > when the associated credentials are unable to log into the remote > > > system. Did the user provide the correct testing credentials, but > > > accidentally typed the production endpoint? Even if the production > > > endpoint looks valid (it's a real hostname that is reachable) the > > > conceptual error is still in the hostname and should have the error > > > attributed to it to draw the user's attention. > > > If that's not convincing, I think the alternative of only attributing > > > version errors to the version property is also acceptable. > > > > > > Thanks, > > > Greg > > > > > > On Wed, May 15, 2024 at 8:06 AM Chris Egerton <chr...@aiven.io.invalid > > > > > wrote: > > > > > > > > Hi Greg, > > > > > > > > Thanks for your responses! Continuations of existing discussions: > > > > > > > > Regarding crashing the worker on startup--yes, there is also a risk > to > > > > allowing it to join the cluster. But this risk is already present > with > > > > existing error cases, and I don't see anything that justifies > changing > > > > existing behavior with an invalid converter class, or diverging from > it > > > in > > > > the case of invalid converter versions. I think we should keep this > > > simple > > > > and not do anything different for worker startup than we do already. > > > > > > > > As far as REST API vs. metrics go--I think you're right that the > original > > > > version metrics were added as a "monitoring" detail. However, this > was > > > back > > > > when plugin versions were managed solely by cluster administrators. > With > > > > this KIP, connector users will be able to manage plugin versions, > and CLI > > > > and programmatic UI developers will want to develop their own tooling > > > > layers. I think focusing on the REST API as the primary interface for > > > this > > > > KIP would be best for these users. > > > > > > > > (All that said, I don't object to the metrics that are proposed in > the > > > KIP; > > > > I just think they make more sense in addition to new REST API > > > > functionality, as opposed to instead of it.) > > > > > > > > Regarding the GET /connector-plugins/<plugin>/config endpoint, I was > > > > thinking about the response for non-connector plugins, e.g., > > > > GET /connector-plugins/RegexRouter/config. Would a "version" property > > > > appear with recommended values? > > > > > > > > > > > > And new thoughts: > > > > > > > > 1) Regarding the recommended values for "connector.version", this > might > > > be > > > > confusing since there could be differences between the ConfigDefs > for the > > > > latest version of the connector and prior versions. It also makes the > > > flow > > > > a bit awkward for programmatic UI developers: if a user changes the > > > > connector version in, e.g., a dropdown menu, then the UI either has > to > > > > re-fetch the ConfigDef for the new version, or risk operating on > stale > > > > information. I'm starting to doubt that exposing the range of > available > > > > versions via recommended values is the best way to proceed, instead > of a > > > > more explicit approach like GET > /connector-plugins/<plugin>/versions, or > > > > the "Adding new REST API endpoints" rejected alternative. > > > > > > > > 2) I know that this is a bit heinous, but will it be possible to > > > configure > > > > a connector to use two instances of the same transform or predicate, > but > > > > with different versions for each? (I don't think this is worth > > > significant > > > > design/implementation effort, so if it would inflate either of those, > > > > please don't feel obligated to support it.) > > > > > > > > 3) Just out of curiosity, why double-ding during config validation > if the > > > > version for a plugin class can't be found? I would have expected the > > > error > > > > to only be attributed to the version property, and for the class > property > > > > to be reported as valid. > > > > > > > > Cheers, > > > > > > > > Chris > > > > > > > > On Tue, May 14, 2024 at 1:11 PM Greg Harris > <greg.har...@aiven.io.invalid > > > > > > > > wrote: > > > > > > > > > Hey Chris, > > > > > > > > > > Thanks for your comments. I'm glad you like the motivations, > Snehashis > > > > > wrote that part! > > > > > > > > > > > the configuration syntax for the most basic use case of > > > > > > specifying a single desired version is pretty counterintuitive. > > > > > > > > > > I agree, and the "soft" requirement scheme is something I wasn't > > > > > explicitly looking for, but would be inherited from the library. > I'm > > > > > fine with eliminating the soft requirement semantics, and having > > > > > "1.0.0" behave the same as "[1.0.0]". I'm less inclined to include > > > > > "range" in the property name, or have two properties. > > > > > > > > > > > Failing startup is drastic and has the potential to disrupt the > > > > > > availability of connectors that would otherwise be able to run > > > healthily > > > > > > because they were explicitly configured to use valid converters > > > instead > > > > > of > > > > > > the worker defaults. > > > > > > > > > > I think this argument cuts both ways. If someone reconfigures a > worker > > > > > and adds an invalid ".version" string to the worker (or changes the > > > > > plugin.path to make it invalid), it would be permitted to enter the > > > > > group, and accept work assignments. If those work assignments used > > > > > these configurations, a set of tasks could transition to FAILED and > > > > > not be able to recover, because they would be restarted again on > the > > > > > same worker. > > > > > > > > > > > Why are metrics utilized to report information about plugin > versions > > > > > > utilized by connectors at runtime instead of publishing this > info in > > > the > > > > > > REST API > > > > > > > > > > I was following the existing pattern for exposing runtime versions > for > > > > > connectors, and it did seem like a "monitoring" feature. If that > > > > > approach is flawed and should be phased out, I think it would be a > > > > > good idea to reconsider the REST API rejected alternative. > > > > > We would need some additional design work to spec out the REST API > > > > > interface, as I don't have anything in mind currently. > > > > > > > > > > > I'm unclear on whether or not it'll be possible to see this > > > information > > > > > via the > > > > > > GET /connector-plugins/<plugin>/config endpoint > > > > > > > > > > There is room in the API to add recommenders for "key.converter", > > > > > "value.converter", and "header.converter", but not for transforms > and > > > > > predicates, as they include aliases that depend on an actual > > > > > configuration. We could explicitly say we're going to do that, or > do > > > > > whatever is convenient during the implementation phase, or leave it > > > > > open to be improved later. > > > > > There will not be any recommenders for ".version" properties in the > > > > > `/config` endpoint, because those recommenders are dynamic and > depend > > > > > on an actual configuration. > > > > > > > > > > 5) There are two relevant lines in the KIP: "If a .version property > > > > > contains a hard requirement, select the latest installed version > which > > > > > satisfies the requirement." and "This configuration is re-evaluated > > > > > each time the connector or task are assigned to a new worker". I > would > > > > > call this "eager" upgrade behavior, rather than a "sticky" or > "lazy" > > > > > upgrade behavior. > > > > > > > > > > 6) Updated! > > > > > > > > > > Thanks, > > > > > Greg > > > > > > > > > > On Tue, May 14, 2024 at 9:14 AM Chris Egerton > <chr...@aiven.io.invalid > > > > > > > > > wrote: > > > > > > > > > > > > Hi all, > > > > > > > > > > > > Thanks Greg for updating the KIP, and thanks Snehashis for > starting > > > the > > > > > > work on this originally. > > > > > > > > > > > > The motivation section makes a pretty convincing case for this > kind > > > of > > > > > > feature. My thoughts are mostly about specific details: > > > > > > > > > > > > 1) I like the support for version ranges (the example > demonstrating > > > how > > > > > to > > > > > > avoid KAFKA-10574 with the header converter was particularly > > > > > entertaining), > > > > > > but the configuration syntax for the most basic use case of > > > specifying a > > > > > > single desired version is pretty counterintuitive. People may get > > > bitten > > > > > or > > > > > > at least frustrated if they put connector.version=3.8.0 in a > config > > > but > > > > > > then version 3.7.5 ends up running. I'd like it if we could > either > > > > > > intentionally deviate from Maven ranges when a bare version is > > > present, > > > > > or > > > > > > separate things out into two properties: foo.version would be the > > > single > > > > > > accepted version for the foo plugin, and foo.version.range would > use > > > > > Maven > > > > > > range syntax. Open to other options too, just providing a couple > to > > > get > > > > > the > > > > > > ball rolling. > > > > > > > > > > > > 2) Although the current behavior for a worker with an invalid > > > > > > key/value/header converter class specified in its config file is > a > > > little > > > > > > strange (I was surprised to learn that it wouldn't fail on > startup), > > > I > > > > > > don't see a good reason to deviate from this when an invalid > version > > > is > > > > > > specified. Failing startup is drastic and has the potential to > > > disrupt > > > > > the > > > > > > availability of connectors that would otherwise be able to run > > > healthily > > > > > > because they were explicitly configured to use valid converters > > > instead > > > > > of > > > > > > the worker defaults. > > > > > > > > > > > > 3) Why are metrics utilized to report information about plugin > > > versions > > > > > > utilized by connectors at runtime instead of publishing this > info in > > > the > > > > > > REST API? I saw that this was mentioned as a rejected > alternative, > > > but I > > > > > > didn't get a sense of why. It seems like the REST API would be > > > easier to > > > > > > access and more intuitive for most users than new metrics. > > > > > > > > > > > > 4) In the "Validation" section it's stated that "Users can use > these > > > > > > recommenders to discover the valid plugin classes and versions, > > > without > > > > > > requiring an earlier call to GET > > > > > /connector-plugins?connectorsOnly=false." > > > > > > I really like the creativity and simplicity of reusing the > > > recommender > > > > > > mechanism to expose available versions for plugins via the REST > API. > > > I'm > > > > > > unclear on whether or not it'll be possible to see this > information > > > via > > > > > the > > > > > > GET /connector-plugins/<plugin>/config endpoint, though. It'd be > > > great if > > > > > > this were supported, since we learned in KIP-769 [1] that people > > > really > > > > > > want to be able to see configuration options for connectors and > their > > > > > > plugins via some kind of GET endpoint without having to provide a > > > > > complete > > > > > > connector config for validation. > > > > > > > > > > > > 5) In the Maven version range docs, it's stated that "Maven > picks the > > > > > > highest version of each project that satisfies all the hard > > > requirements > > > > > of > > > > > > the dependencies on that project." I'm guessing this behavior > will be > > > > > > retained for Connect; i.e., the highest-possible version of each > > > plugin > > > > > > that satisfies the user-specified version constraints will be > run? > > > (An > > > > > > alternative approach could be to have some kind of "sticky" logic > > > that > > > > > only > > > > > > restarts connectors/tasks when their currently-used version > becomes > > > > > > incompatible with the configured constraints.) > > > > > > > > > > > > 6) (Nit) It'd be nice to add a link to the TestPlugins class or > > > somewhere > > > > > > in its neighborhood to the testing plan; unfamiliar readers > probably > > > > > won't > > > > > > get much out of what's there right now. > > > > > > > > > > > > [1] - > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+APIs+to+list+all+connector+plugins+and+retrieve+their+configuration+definitions > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Chris > > > > > > > > > > > > On Mon, May 13, 2024 at 2:01 PM Snehashis < > snehashisp1...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > Hi Greg, > > > > > > > > > > > > > > That is much appreciated. No complaints on the additional > scope, I > > > will > > > > > > > make some time out to work on this once we have approval. > > > > > > > > > > > > > > Thanks > > > > > > > Snehashis > > > > > > > > > > > > > > On Fri, May 10, 2024 at 9:28 PM Greg Harris > > > > > <greg.har...@aiven.io.invalid> > > > > > > > wrote: > > > > > > > > > > > > > > > Hey Snehashis, > > > > > > > > > > > > > > > > I'm glad to hear you're still interested in this KIP! > > > > > > > > I'm happy to let you drive this, and I apologize for > increasing > > > the > > > > > > > > scope of work so drastically. To make up for that, I'll > > > volunteer to > > > > > > > > be the primary PR reviewer to help get this done quickly once > > > the KIP > > > > > > > > is approved. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Greg > > > > > > > > > > > > > > > > > > > > > > > > On Fri, May 10, 2024 at 3:51 AM Snehashis < > > > snehashisp1...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Greg, > > > > > > > > > > > > > > > > > > Thanks for the follow up to my original KIP, I am in > favour of > > > the > > > > > > > > > additions made to expand its scope, the addition of range > > > versions > > > > > > > > > specifically make a lot of sense. > > > > > > > > > > > > > > > > > > Apologies if I have not publicly worked on this KIP for a > long > > > > > time. > > > > > > > The > > > > > > > > > original work was done when the move to service loading > was in > > > > > > > discussion > > > > > > > > > and I wanted to loop back to this only after that work was > > > > > completed. > > > > > > > > Post > > > > > > > > > its conclusion, I have not been able to take this up due to > > > other > > > > > > > > > priorities. If it's okay with you, I would still like to > get > > > this > > > > > > > > > implemented myself, including the additional scope. > > > > > > > > > > > > > > > > > > Thanks and regards > > > > > > > > > Snehashis > > > > > > > > > > > > > > > > > > On Fri, May 10, 2024 at 12:45 AM Greg Harris > > > > > > > > <greg.har...@aiven.io.invalid> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > I'd like to reboot the discussion on KIP-891: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-891%3A+Running+multiple+versions+of+Connector+plugins > > > > > > > > > > > > > > > > > > > > I've made some changes, most notably: > > > > > > > > > > > > > > > > > > > > 1. Specifying versions for all plugins in Connector > configs > > > > > > > > > > (converters, header converters, transforms, and > predicates) > > > not > > > > > just > > > > > > > > > > connectors & tasks > > > > > > > > > > 2. Specifying a range of versions instead of an exact > match > > > > > > > > > > 3. New metrics to observe what versions are in-use > > > > > > > > > > > > > > > > > > > > Thanks to Snehashis for the original KIP idea! > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Greg > > > > > > > > > > > > > > > > > > > > On Tue, Jan 2, 2024 at 11:49 AM Greg Harris < > > > > > greg.har...@aiven.io> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Snehashis, > > > > > > > > > > > > > > > > > > > > > > Thank you for the KIP! This is something I've wanted > for a > > > long > > > > > > > time. > > > > > > > > > > > > > > > > > > > > > > I know the discussion has gone cold, are you still > > > interested > > > > > in > > > > > > > > > > > pursuing this feature? I'll make time to review the > KIP if > > > you > > > > > are > > > > > > > > > > > still accepting comments. > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Greg > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 12:29 PM Snehashis < > > > > > > > snehashisp1...@gmail.com > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the points Sagar. > > > > > > > > > > > > > > > > > > > > > > > > > 1) Should we update the GET /connectors endpoint to > > > > > include the > > > > > > > > > > version of > > > > > > > > > > > > > the plugin that is running? It could be useful to > > > figure > > > > > out > > > > > > > the > > > > > > > > > > version > > > > > > > > > > > > of > > > > > > > > > > > > > the plugin or I am assuming it gets returned by the > > > > > expand=info > > > > > > > > call? > > > > > > > > > > > > > > > > > > > > > > > > I think this is good to have and possible future > > > > > enhancement. The > > > > > > > > > > version > > > > > > > > > > > > info will be present in the config of the connector > if > > > the > > > > > user > > > > > > > has > > > > > > > > > > > > specified the version. Otherwise it is the latest > version > > > > > which > > > > > > > the > > > > > > > > > > user > > > > > > > > > > > > can find out from the connector-plugin endpoint. The > > > > > information > > > > > > > > can be > > > > > > > > > > > > introduced to the response of the GET /connectors > > > endpoint > > > > > > > itself, > > > > > > > > > > however > > > > > > > > > > > > the most ideal way of doing this would be to get the > > > > > currently > > > > > > > > running > > > > > > > > > > > > instance of the connector and get the version > directly > > > from > > > > > > > there. > > > > > > > > > > This is > > > > > > > > > > > > slightly tricky as the connector could be running in > a > > > > > different > > > > > > > > node. > > > > > > > > > > > > One way to do this would be to persist the version > > > > > information in > > > > > > > > the > > > > > > > > > > > > status backing store during instantiation of the > > > connector. > > > > > It > > > > > > > > requires > > > > > > > > > > > > some more thought and since the version is part of > the > > > > > configs if > > > > > > > > > > provided > > > > > > > > > > > > and evident otherwise, I have not included it in this > > > KIP. > > > > > > > > > > > > > > > > > > > > > > > > > 2) I am not aware of this and hence asking, can 2 > > > > > connectors > > > > > > > with > > > > > > > > > > > > different > > > > > > > > > > > > > versions have the same name? Does the plugin > isolation > > > > > allow > > > > > > > > this? > > > > > > > > > > This > > > > > > > > > > > > > could have a bearing when using the lifecycle > > > endpoints for > > > > > > > > > > connectors > > > > > > > > > > > > like > > > > > > > > > > > > > DELETE etc. > > > > > > > > > > > > > > > > > > > > > > > > All connectors in a cluster need to have uniquire > > > connector > > > > > names > > > > > > > > > > > > regardless of what version of the plugin the > connector is > > > > > running > > > > > > > > > > > > underneath. This is something enforced by the connect > > > runtime > > > > > > > > itself. > > > > > > > > > > All > > > > > > > > > > > > connect CRUD operations are keyed on the connector > name > > > so > > > > > there > > > > > > > > will > > > > > > > > > > not > > > > > > > > > > > > be an issue. > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > > > > Snehashis > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 3:16 PM Sagar < > > > > > sagarmeansoc...@gmail.com > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hey Snehashsih, > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP. It looks like a very useful > > > feature. > > > > > Couple > > > > > > > > of > > > > > > > > > > > > > small-ish points, let me know what you think: > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Should we update the GET /connectors endpoint to > > > > > include the > > > > > > > > > > version of > > > > > > > > > > > > > the plugin that is running? It could be useful to > > > figure > > > > > out > > > > > > > the > > > > > > > > > > version of > > > > > > > > > > > > > the plugin or I am assuming it gets returned by the > > > > > expand=info > > > > > > > > call? > > > > > > > > > > > > > 2) I am not aware of this and hence asking, can 2 > > > > > connectors > > > > > > > with > > > > > > > > > > different > > > > > > > > > > > > > versions have the same name? Does the plugin > isolation > > > > > allow > > > > > > > > this? > > > > > > > > > > This > > > > > > > > > > > > > could have a bearing when using the lifecycle > > > endpoints for > > > > > > > > > > connectors like > > > > > > > > > > > > > DELETE etc. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > Sagar. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 2:10 PM Ashwin > > > > > > > > <apan...@confluent.io.invalid > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Snehasis, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > IIUC (please correct me if I am wrong here), > what > > > you > > > > > > > > highlighted > > > > > > > > > > > > > above, > > > > > > > > > > > > > > is > > > > > > > > > > > > > > a versioning scheme for a connector config for > the > > > same > > > > > > > > connector > > > > > > > > > > (and > > > > > > > > > > > > > not > > > > > > > > > > > > > > different versions of a connector plugin). > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry for not being more precise in my wording > - I > > > meant > > > > > > > > > > registering > > > > > > > > > > > > > > versions of schema for connector config. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Let's take the example of a fictional connector > which > > > > > uses a > > > > > > > > > > fictional > > > > > > > > > > > > > AWS > > > > > > > > > > > > > > service. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fictional Connector Config schema version:2.0 > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > { > > > > > > > > > > > > > > "$schema": " > > > http://json-schema.org/draft-04/schema#";, > > > > > > > > > > > > > > "type": "object", > > > > > > > > > > > > > > "properties": { > > > > > > > > > > > > > > "name": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "schema_version": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "aws_access_key": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "aws_secret_key": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > } > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "required": [ > > > > > > > > > > > > > > "name", > > > > > > > > > > > > > > "schema_version", > > > > > > > > > > > > > > "aws_access_key", > > > > > > > > > > > > > > "aws_secret_key" > > > > > > > > > > > > > > ] > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fictional Connector config schema version:3.0 > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > { > > > > > > > > > > > > > > "$schema": " > > > http://json-schema.org/draft-04/schema#";, > > > > > > > > > > > > > > "type": "object", > > > > > > > > > > > > > > "properties": { > > > > > > > > > > > > > > "name": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "schema_version": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "iam_role": { > > > > > > > > > > > > > > "type": "string" > > > > > > > > > > > > > > } > > > > > > > > > > > > > > }, > > > > > > > > > > > > > > "required": [ > > > > > > > > > > > > > > "name", > > > > > > > > > > > > > > "schema_version", > > > > > > > > > > > > > > "iam_role" > > > > > > > > > > > > > > ] > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > The connector which supports Fictional config > schema > > > 2.0 > > > > > > > will > > > > > > > > > > validate > > > > > > > > > > > > > the > > > > > > > > > > > > > > access key and secret key. > > > > > > > > > > > > > > Whereas a connector which supports config with > schema > > > > > version > > > > > > > > 3.0 > > > > > > > > > > will > > > > > > > > > > > > > only > > > > > > > > > > > > > > validate the IAM role. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is the alternative which I wanted to > suggest. > > > Each > > > > > > > plugin > > > > > > > > will > > > > > > > > > > > > > > register the schema versions of connector config > > > which it > > > > > > > > supports. > > > > > > > > > > > > > > > > > > > > > > > > > > > > The plugin paths may be optionally different > i.e we > > > > > don't > > > > > > > > have to > > > > > > > > > > > > > > mandatorily add a new plugin path to support a > new > > > schema > > > > > > > > version. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Ashwin > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 12:47 PM Snehashis < > > > > > > > > > > snehashisp1...@gmail.com> > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the input Ashwin. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. Can you elaborate on the rejected > > > alternatives ? > > > > > > > Suppose > > > > > > > > > > connector > > > > > > > > > > > > > > > > config is versioned and has a schema. Then a > > > single > > > > > > > plugin > > > > > > > > > > (whose > > > > > > > > > > > > > > > > dependencies have not changed) can handle > > > multiple > > > > > config > > > > > > > > > > versions > > > > > > > > > > > > > for > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > same connector class. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > IIUC (please correct me if I am wrong here), > what > > > you > > > > > > > > highlighted > > > > > > > > > > > > > above, > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > a versioning scheme for a connector config for > the > > > same > > > > > > > > > > connector (and > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > different versions of a connector plugin). > That is > > > a > > > > > > > somewhat > > > > > > > > > > > > > tangential > > > > > > > > > > > > > > > problem. While it is definitely a useful > feature to > > > > > have, > > > > > > > > like a > > > > > > > > > > log to > > > > > > > > > > > > > > > check what changes were made over time to the > > > config > > > > > which > > > > > > > > might > > > > > > > > > > make > > > > > > > > > > > > > it > > > > > > > > > > > > > > > easier to do rollbacks, it is not the focus > here. > > > Here > > > > > by > > > > > > > > > > version we > > > > > > > > > > > > > mean > > > > > > > > > > > > > > > to say what underlying version of the plugin > > > should the > > > > > > > given > > > > > > > > > > > > > > configuration > > > > > > > > > > > > > > > of the connector use. Perhaps it is better to > > > change > > > > > the > > > > > > > > name of > > > > > > > > > > the > > > > > > > > > > > > > > > parameter from connector.version to > > > > > > > connector.plugin.version > > > > > > > > or > > > > > > > > > > > > > > > plugin.version if it was confusing. wdyt? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. Any plans to support assisted migration > e.g > > > if a > > > > > user > > > > > > > > > > invokes > > > > > > > > > > > > > "POST > > > > > > > > > > > > > > > > connector/config?migrate=latest", the latest > > > version > > > > > > > > > > __attempts__ to > > > > > > > > > > > > > > > > transform the existing config to the newer > > > version. > > > > > This > > > > > > > > would > > > > > > > > > > > > > require > > > > > > > > > > > > > > > > adding a method like "boolean migrate(Version > > > > > > > > fromVersion)" to > > > > > > > > > > the > > > > > > > > > > > > > > > > connector interface. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is an enhancement we can think of doing in > > > future. > > > > > > > > Users can > > > > > > > > > > > > > simply > > > > > > > > > > > > > > do > > > > > > > > > > > > > > > a PUT call with the updated config which has > the > > > > > updated > > > > > > > > version > > > > > > > > > > > > > number. > > > > > > > > > > > > > > > The assisted mode could be handy as the user > does > > > not > > > > > need > > > > > > > to > > > > > > > > > > know the > > > > > > > > > > > > > > > config but beyond this it does not seem to > justify > > > its > > > > > > > > existence. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > > > > > > > Snehashis > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 10:50 AM Ashwin > > > > > > > > > > <apan...@confluent.io.invalid> > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Snehasis, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is a really useful feature and thanks > for > > > > > initiating > > > > > > > > this > > > > > > > > > > > > > > > discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I had the following questions - > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. Can you elaborate on the rejected > > > alternatives ? > > > > > > > Suppose > > > > > > > > > > connector > > > > > > > > > > > > > > > > config is versioned and has a schema. Then a > > > single > > > > > > > plugin > > > > > > > > > > (whose > > > > > > > > > > > > > > > > dependencies have not changed) can handle > > > multiple > > > > > config > > > > > > > > > > versions > > > > > > > > > > > > > for > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > same connector class. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. Any plans to support assisted migration > e.g > > > if a > > > > > user > > > > > > > > > > invokes > > > > > > > > > > > > > "POST > > > > > > > > > > > > > > > > connector/config?migrate=latest", the latest > > > version > > > > > > > > > > __attempts__ to > > > > > > > > > > > > > > > > transform the existing config to the newer > > > version. > > > > > This > > > > > > > > would > > > > > > > > > > > > > require > > > > > > > > > > > > > > > > adding a method like "boolean migrate(Version > > > > > > > > fromVersion)" to > > > > > > > > > > the > > > > > > > > > > > > > > > > connector interface. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Ashwin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 21, 2022 at 2:27 PM Snehashis < > > > > > > > > > > snehashisp1...@gmail.com> > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'd like to start a discussion thread on > > > KIP-891: > > > > > > > Running > > > > > > > > > > multiple > > > > > > > > > > > > > > > > versions > > > > > > > > > > > > > > > > > of a connector. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The KIP aims to add the ability for the > connect > > > > > runtime > > > > > > > > to > > > > > > > > > > run > > > > > > > > > > > > > > multiple > > > > > > > > > > > > > > > > > versions of a connector. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-891%3A+Running+multiple+versions+of+a+connector > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please take a look and let me know what you > > > think. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you > > > > > > > > > > > > > > > > > Snehashis Pal > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
