Thanks Chris and Tom for the feedback. I'm having a hard time making a decision on this point!
Re-reading your previous comments, I agree that we're lacking a contract on the version field, and we can see value (supporting multiple versions for example) if this was improved. But I'm starting to wonder if this should be handled in a different KIP. In this KIP, we can keep the existing Versioned interface for Connectors and either not display a version for other plugins or hard-code it to something like "N/A". Later if we think we're missing the version for other plugins we can decide how to handle it. While it would have been nice to harmonize all plugins, I don't think it's a requirement or blocker for the rest of the value of this KIP. Unless you disagree and feel like we need versions for all plugins, I'll update the KIP accordingly. Thanks, Mickael On Thu, Jan 27, 2022 at 9:00 PM Tom Bentley <tbent...@redhat.com> wrote: > > To be honest I'm very sceptical that you can evolve (without breaking > compatibility) from a situation of having a large number of plugins which > have already implemented version() to return whatever they please, to a > situation where you can rely on it as the basis for some feature to support > multiple versions of plugins. It would be much safer to introduce a new > versioning concept, perhaps using Jar manifests and > "Specification-Version", than to try to build on Versioned. > > So I would prefer Javadoc that clearly said that this version number was a > means for the plugin author to identify the version of their plugin in use > by users, for the purpose of bug reporting. That at least makes is clear > that Connect doesn't do anything with it except pass it through to the API. > It also makes clear that using "undefined" is a perfectly fine > implementation for PossiblyVersioned to have. > > Kind regards, > > Tom > > > > > > > On Thu, 27 Jan 2022 at 18:37, Chris Egerton <chr...@confluent.io.invalid> > wrote: > > > Hi Mickael, > > > > +1 for the warning at startup, it'd be great if we could get everyone to > > start versioning all of their components. Although given the current state > > of WARN-level logging in Connect (see > > https://issues.apache.org/jira/browse/KAFKA-7509) there might be too much > > noise for most people to notice, unfortunately. Still, better than nothing! > > > > I'm not sure it'd be wise to move quite so quickly with deprecating and > > then removing the PossiblyVersioned interface. In an ideal world everyone > > would update their converters, SMTs, and predicates as soon as they saw the > > warning, but there could be some plugins out there that aren't maintained > > anymore and would become incompatible with a Connect runtime that requires > > them to provide versions. I can imagine there might be some fairly trivial > > but valuable SMTs or converters that were written years ago and haven't > > been touched in some time but are still useful to Connect users today. > > > > I think it'd be fine to log a scary-looking warning message stating that > > the plugin may be incompatible with future versions of Connect, but perhaps > > we can leave the decision about how and when we actually create that > > incompatibility to a future KIP. If we take the example use case of loading > > multiple versions of the same plugin on the same worker, for example, that > > feature would obviously require plugins to provide versions, but we could > > potentially add some failsafe logic in order to still support versionless > > plugins with the understanding that it would not be possible to colocate > > multiple versions of them on the same worker. > > > > Cheers, > > > > Chris > > > > On Thu, Jan 27, 2022 at 7:43 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Hi Chris, > > > > > > Thanks for taking another look! > > > > > > That's a good point which I should have mentioned in the KIP. I think > > > we can have a PossiblyVersioned interface that extends Versioned and > > > has the default implementation. I'd like to also print a warning at > > > startup if a plugin does not override version(). Thanks for the > > > suggestion! > > > > > > I'm even considering whether PossiblyVersioned could be deprecated > > > immediately in order to be removed in the next major release. If so, > > > the warning message I mentioned would state that plugins not > > > overriding version() would stop working in the next major release. > > > > > > Thanks, > > > Mickael > > > > > > >
