Skip to content

Check Connector version in SPI compatibility check#11951

Closed
findepi wants to merge 3 commits intotrinodb:masterfrom
findepi:findepi/spi-version-new
Closed

Check Connector version in SPI compatibility check#11951
findepi wants to merge 3 commits intotrinodb:masterfrom
findepi:findepi/spi-version-new

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Apr 14, 2022

Instead of checking ConnectorFactory version, check the actual
Connector implementation class's version. The Connector
implementation is the thing which fulfills SPI contract, so it is a more
appropriate thing to test.

Follow up to #11774

@findepi findepi added maintenance Project maintenance task no-release-notes This pull request does not require release notes entry labels Apr 14, 2022
@cla-bot cla-bot bot added the cla-signed label Apr 14, 2022
Copy link
Copy Markdown
Member

@leveyja leveyja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if this helps or just makes it harder for consumers to uptake new Trino release - I would be happier if tests were added, also).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "maven" version? This is Return the version of this implementation. It consists of any string assigned by the vendor of this implementation and does not have any particular syntax specified or expected by the Java runtime.

I think this is independent of Maven and the name should be changed to not reflect the particular build system (again, this is a plugin-toolkit, expected to be used/understood by consumers who don't use "Maven").

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's Trino code. All Trino plugins use airbase and this depends on airbase's populating jar's manifest.
A non-Trino (3rd party) plugin should probably not call this method.

(BTW I agree the original change (#11774) could use a better source of information -- the SPI version a connector was compiled against -- instead of connector's version. However, i didn't find a way to do this without changing each of the connectors. From Trino project's perspective, it would be an unnecessary complexity anyway, since SPI version is the same as the connector's version)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really easy to do (if you know how, and luckily there is prior art)

Connectors which want an SPI version match should have a Maven Transformation in their pom which writes some "attribute" to the Manifest, and, if present, Trino enforces a match.

And we write tests for the match. And we add a flag to disable the match (for CI, or testing upgrades without having to change versions, etc...).

Instead of checking `ConnectorFactory` version, check the actual
`Connector` implementation class's version. The `Connector`
implementation is the thing which fulfills SPI contract, so it is a more
appropriate thing to test.
@findepi findepi force-pushed the findepi/spi-version-new branch from 887a6ee to 2a9e35a Compare April 14, 2022 14:36
@findepi findepi requested a review from leveyja April 14, 2022 15:27
Copy link
Copy Markdown
Member

@leveyja leveyja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see tests or documentation to explain the rules and intended usage of checkSpiVersion (it's public static and appears to be intended for use by developers of plugins, so it will become important that the usage and quirks are understood - right now I can't tell what effect it has on developing a -SNAPSHOT plugin against Trino release (or testing a released plugin against a Trino snapshot, etc)).

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 14, 2022

The method is documented.

It cannot be really tested easily, as it depends on the build system. True testing would require product tests which i believe would be an overkill here. The fact that the method allows correct SPI/plugin usage is covered by product tests implicitly, so the worst thing that can happen and is not test-covered is that the method doesn't prevent an incorrect SPI/plugin usage, and this is little damage, an acceptable risk.

@findepi findepi closed this Apr 15, 2022
@findepi findepi deleted the findepi/spi-version-new branch April 15, 2022 11:36
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 19, 2022

Another iteration: #12006

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants