I think a simple approach here is fine. +1 for removing versioning for other plugins.
On Fri, Jan 28, 2022 at 11:17 AM Mickael Maison <mickael.mai...@gmail.com> wrote: > 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 > > > > > > > > > > >
