Skip to content

Relax SPI compatibility requirements#18072

Closed
ppalucha wants to merge 1 commit intotrinodb:masterfrom
ppalucha:pp/relax-spi-check
Closed

Relax SPI compatibility requirements#18072
ppalucha wants to merge 1 commit intotrinodb:masterfrom
ppalucha:pp/relax-spi-check

Conversation

@ppalucha
Copy link
Member

Description

Check only if major version matches when performing SPI version check.

Additional context and related issues

This change allows to test snapshot version of plugin against the released Trino version.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Relax SPI version check for plugins, so we can check snapshot plugins versions against the released Trino version.

@cla-bot cla-bot bot added the cla-signed label Jun 28, 2023
@ppalucha ppalucha requested review from bitweld, findepi and kokosing June 28, 2023 10:15
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comments

Copy link
Member

Choose a reason for hiding this comment

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

assertFalse(Versions.checkMatch("420", "421"));
assertFalse(Versions.checkMatch("420", "421-SNAPSHOT"));

Copy link
Member Author

Choose a reason for hiding this comment

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

I messed it up, both no snapshots is already covered in previous test case. Should be good now.

Check only if major version matches. This allows to test snapshot
version of plugin against released Trino version.
@martint
Copy link
Member

martint commented Jun 28, 2023

What's the use case for this?

Copy link
Member

@bitweld bitweld left a comment

Choose a reason for hiding this comment

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

LGTM

@kokosing
Copy link
Member

What's the use case for this?

@martint as stated in commit message This allows to test snapshot version of plugin against released Trino version.. This change allows 3rd party plugins (there are plenty of them in the wild) that are under development to use this check. Then once the plugins are released it is ensured that they are used with proper Trino version.

@martint
Copy link
Member

martint commented Jun 28, 2023

I read that, but I don't understand how this helps third-party plugins. A third party plugin under development would be referencing a release version of Trino regardless of what version the plugin has (the version of the plugin is unrelated to Trino if it's not part of the Trino codebase). A plugin within the Trino tree would not have this problem, as its version would already match.

So the question I'm asking is what is the specific use case for this change, as it's not at all clear from the description.

@kokosing
Copy link
Member

Thank you @martint for the feedback. I forced me to think me more about it. Actually with 3rd party plugins we cannot assume any versioning pattern. So relaxing this does not make much sense. I think people should not use this function outside of Trino.

@kokosing kokosing closed this Jun 29, 2023
@findepi
Copy link
Member

findepi commented Jul 3, 2023

A 3rd party plugin with specific versioning compatibility should not invoke Versions.checkSpiVersion

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

Development

Successfully merging this pull request may close these issues.

5 participants