Enforce connector and SPI version match for Trino-maintained plugins by findepi · Pull Request #11774 · trinodb/trino

findepi · 2022-04-04T09:15:29Z

While SPI intends to be backwards compatible, the plugins bundled with
the project do not intend to be compatible with any other engine or SPI
version than the ones they were build for. Sometimes users end up
deploying a plugin version that does not match the SPI version. This
can lead to hard to explain exceptions or even correctness problems,
especially if the connector relies on an engine feature that is not
present.

This commit adds version checks to all plugins bundled with the project.

findepi · 2022-04-04T09:18:15Z

cc @nineinchnick

core/trino-main/src/main/java/io/trino/connector/ConnectorContextInstance.java

nineinchnick

As an author of a few third-party plugins, I know this will cause me more work but I think it's a good idea. I was never sure the server will start up correctly, and now I'd know I have to update all my plugins.

I'd probably need to adjust the versioning scheme of my plugins to be more in sync with Trino, but that'd also make some things easier in the long term.

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java

plugin/trino-accumulo/src/main/java/io/trino/plugin/accumulo/AccumuloConnectorFactory.java

ssheikin · 2022-04-04T09:32:15Z

@findepi what if the plugin used with trino is a proprietary one with custom release schedule?

cc @lukasz-walkiewicz

findepi · 2022-04-04T09:42:06Z

As an author of a few third-party plugins, I know this will cause me more work but I think it's a good idea. I was never sure the server will start up correctly, and now I'd know I have to update all my plugins.

what if the plugin used with trino is a proprietary one with custom release schedule?

@nineinchnick @ssheikin @lukasz-walkiewicz

the check is done on the plugin side. There is no change for plugins maintained outside of this repository.
The 3rd party plugins MAY start using such checks (and I encourage everyone to do so), but are free not to do so.

ssheikin · 2022-04-04T09:44:51Z

@nineinchnick, @hashhar you were faster :)

I'd probably need to adjust the versioning scheme of my plugins to be more in sync with Trino, but that'd also make some things easier in the long term.

I guess that would cause significant delays for upgrade of some trino installations which use proprietary plugins which in their turn relies on some other proprietary plugins. And now all that plugins must use the same trino SPI version now.

May be we have to enforce some window for backward compatible SPI versions?
cc @kokosing @lukasz-walkiewicz

findepi · 2022-04-04T09:48:16Z

@ssheikin i still don't understand your concerns. This adds checks to plugins maintained here, not to some proprietary plugins built and used somewhere.

ssheikin · 2022-04-04T09:50:02Z

@findepi Thank you for clarification. Sounds reasonable.

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java

losipiuk

LGTM

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java

hashhar

Looks good to me (since it's plugin's choice to opt-into this).

plugin/trino-tpcds/pom.xml

findepi · 2022-04-05T07:15:08Z

The build is failing miserably, while it passes locally. I think it may be either related to merge commits, or to GIB. Going to investigate this.

Build results depend on `git describe` equivalent (the git-commit-id maven plugin).

findepi · 2022-04-05T13:23:29Z

The build is failing miserably, while it passes locally. I think it may be either related to merge commits, or to GIB. Going to investigate this.

Some CI tasks where still using shallow checkout, so git-commit-id plugin didn't work correctly. I added separate commit to change that.

While SPI intends to be backwards compatible, the plugins bundled with the project do not intend to be compatible with any other engine or SPI version than the ones they were build for. Sometimes users end up deploying a plugin version that does not match the SPI version. This can lead to hard to explain exceptions or even correctness problems, especially if the connector relies on an engine feature that is not present. This commit adds version checks to all plugins bundled with the project.

core/trino-spi/src/main/java/io/trino/spi/connector/SpiVersionHolder.java

findepi · 2022-04-06T09:25:50Z

CI #2330

findepi · 2022-04-06T12:03:08Z

CI #11747

losipiuk · 2022-04-06T17:05:02Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java

+
+        // Implementation version comes from Manifest and is defined in Airbase as equivalent to `git describe` output.
+        if (implementationVersion.matches(".*-(\\d+)-g([0-9a-f]+)(-dirty)?$")) {
+            // Specification version comes from Manifest and is the project version with `-SNAPSHOT` stripped, and .0 added (if not present).


magic... is that contract that will hold?

https://github.com/airlift/airbase/blob/e58ac27e03696460f3cd9ec4400c4454f529121a/airbase/pom.xml#L512-L522

findepi added the no-release-notes This pull request does not require release notes entry label Apr 4, 2022

findepi requested review from a team, dain, electrum, hashhar, kokosing, losipiuk, martint and sopel39 April 4, 2022 09:15

cla-bot bot added the cla-signed label Apr 4, 2022

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from c590101 to 595af95 Compare April 4, 2022 09:28

hashhar reviewed Apr 4, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/connector/ConnectorContextInstance.java Outdated Show resolved Hide resolved

nineinchnick reviewed Apr 4, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java Outdated Show resolved Hide resolved

plugin/trino-accumulo/src/main/java/io/trino/plugin/accumulo/AccumuloConnectorFactory.java Outdated Show resolved Hide resolved

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from 595af95 to 37a502e Compare April 4, 2022 09:43

findepi changed the title ~~Enforce plugin and SPI version match~~ Enforce connector and SPI version match for Trino-maintained plugins Apr 4, 2022

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from 37a502e to 932108b Compare April 4, 2022 09:49

github-actions bot added the tests:hive label Apr 4, 2022

nineinchnick reviewed Apr 4, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java Outdated Show resolved Hide resolved

nineinchnick approved these changes Apr 4, 2022

View reviewed changes

losipiuk reviewed Apr 4, 2022

View reviewed changes

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch 2 times, most recently from dfb02bb to f98fad2 Compare April 4, 2022 12:39

kokosing approved these changes Apr 4, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/Versions.java Outdated Show resolved Hide resolved

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from f98fad2 to a495cf5 Compare April 4, 2022 15:02

findepi requested a review from losipiuk April 4, 2022 15:02

hashhar approved these changes Apr 4, 2022

View reviewed changes

plugin/trino-tpcds/pom.xml Outdated Show resolved Hide resolved

findepi requested review from hashhar, kokosing and nineinchnick April 5, 2022 08:13

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from bdac72d to a947581 Compare April 5, 2022 09:16

Always fetch all commits on CI

d681ba6

Build results depend on `git describe` equivalent (the git-commit-id maven plugin).

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from a947581 to f13588c Compare April 5, 2022 13:22

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from e90ff17 to 90ad2b3 Compare April 5, 2022 14:13

findepi force-pushed the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch from 90ad2b3 to 9d792c5 Compare April 5, 2022 14:20

findepi mentioned this pull request Apr 6, 2022

Verify that no non deprecated SPI is removed #11722

Merged

kokosing approved these changes Apr 6, 2022

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/SpiVersionHolder.java Show resolved Hide resolved

empty

aff42d8

findepi merged commit 664a2bf into trinodb:master Apr 6, 2022

findepi deleted the findepi/enforce-plugin-and-spi-version-match-b1ce30 branch April 6, 2022 15:18

mosabua mentioned this pull request Apr 6, 2022

Add Trino 376 release notes #11691

Merged

github-actions bot added this to the 376 milestone Apr 6, 2022

losipiuk reviewed Apr 6, 2022

View reviewed changes

findepi mentioned this pull request Apr 14, 2022

Check Connector version in SPI compatibility check #11951

Closed

Conversation

findepi commented Apr 4, 2022

Uh oh!

findepi commented Apr 4, 2022

Uh oh!

Uh oh!

nineinchnick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ssheikin commented Apr 4, 2022

Uh oh!

findepi commented Apr 4, 2022

Uh oh!

ssheikin commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Apr 4, 2022

Uh oh!

ssheikin commented Apr 4, 2022

Uh oh!

Uh oh!

losipiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi commented Apr 5, 2022

Uh oh!

findepi commented Apr 5, 2022

Uh oh!

Uh oh!

findepi commented Apr 6, 2022

Uh oh!

findepi commented Apr 6, 2022

Uh oh!

losipiuk Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

findepi Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

ssheikin commented Apr 4, 2022 •

edited

Loading