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