Skip to content

Simplify version logic in plugin/SPI compatibility check#12006

Merged
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/version-spi-3
Apr 20, 2022
Merged

Simplify version logic in plugin/SPI compatibility check#12006
findepi merged 1 commit intotrinodb:masterfrom
findepi:findepi/version-spi-3

Conversation

@findepi
Copy link
Member

@findepi findepi commented Apr 19, 2022

The check relied on the fact that plugin version and SPI version match
for all plugins maintained within Trino. While this is true, loading
plugin version from jar's manifest isn't exactly same as loading the
version from a maven-filtered resource. This in turn lead to
discrepancies in reported version when a tagged version was rebuilt with
local modifications. (Of course, this remains not a recommended
practice).

This commit replaces the jar's manifest and with explicit maven-filtered
resource on the plugin side. The filtered resource is within
trino-plugin-toolkit, so the check still depends on SPI, Trino plugins
and the plugin toolkit being versioned together and so remains
appropriate only for the plugins maintained within the project.

The check relied on the fact that plugin version and SPI version match
for all plugins maintained within Trino. While this is true, loading
plugin version from jar's manifest isn't exactly same as loading the
version from a maven-filtered resource. This in turn lead to
discrepancies in reported version when a tagged version was rebuilt with
local modifications. (Of course, this remains not a recommended
practice).

This commit replaces the jar's manifest and with explicit maven-filtered
resource on the plugin side. The filtered resource is within
trino-plugin-toolkit, so the check still depends on SPI, Trino plugins
and the plugin toolkit being versioned together and so remains
appropriate only for the plugins maintained within the project.
@findepi findepi added maintenance Project maintenance task no-release-notes This pull request does not require release notes entry labels Apr 19, 2022
@cla-bot cla-bot bot added the cla-signed label Apr 19, 2022
@findepi findepi requested a review from wendigo April 19, 2022 10:06
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM though it proved tricky enough times already - so you never know...

Copy link
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.

Apologies, but without a test to show the code being invoked / expected results, this is very hard to reason about.
Also, the compiler will inline a static string - should SPI_VERSION be the result of a method call (not a class initialization?).
Again - without tests to show the expected behaviour, I would be wary of merging this.

Here's an example of some tests / refactoring to make it self-documenting:
master...leveyja:leveyja/trino-spi-version
(and an example of intentionally inlining the version - if we generated a class that allowed Context.getSpiVersion() {return SpiVersionHolder.ACTUAL_STRING_TO_BE_INLINED;}, it could be another avenue (obviously without a value at compile time the current SPI_VERSION can't be inlined - but it might be an avenue to investigate?)

@findepi
Copy link
Member Author

findepi commented Apr 20, 2022

Apologies, but without a test to show the code being invoked / expected results, this is very hard to reason about.

The implemented logic is now requivalent to loading resources plus String.equals call.
While I can add unit tests for String.equals, I didn't find it adding value.

Here's an example of some tests / refactoring to make it self-documenting:
master...leveyja:leveyja/trino-spi-version

Indeed, the code (as currently published on that branch), verifies that String.equals is the intended behavior and that loading info from maven-filtered resource works (it doesn't matter whether the filtered resource is a .java or a .txt, does it?).
Both these things are verified as part of full product run in product tests, and do not add additional value other than documenting what the code does. However, I don't find it necessary: with this change, the "logic" is one-line (checkState(spiVersion.equals(compileTimeSpiVersion),).

Also, the compiler will inline a static string - should SPI_VERSION be the result of a method call (not a class initialization?).

SPI_VERSION is not a "compile-time constant", cannot be inlined.

if we generated a class that allowed Context.getSpiVersion() {return SpiVersionHolder.ACTUAL_STRING_TO_BE_INLINED;}, it could be another avenue (obviously without a value at compile time

I thought about that. Thank you @leveyja for teaching me how to do this, I didn't know that.

For posterity

            <plugin>
                <groupId>com.google.code.maven-replacer-plugin</groupId>
                <artifactId>maven-replacer-plugin</artifactId>
                <version>1.4.0</version>
                <executions>
                    <execution>
                        <phase>process-sources</phase>
                        <goals>
                            <goal>replace</goal>
                        </goals>
                    </execution>
                </executions>
                <configuration>
                    <file>src/main/resources/io/trino/plugin/base/CompileVersionHolder.java.template</file>
                    <outputFile>src/main/java/io/trino/plugin/base/CompileVersionHolder.java</outputFile>
                    <replacements>
                        <replacement>
                            <token>@project.version@</token>
                            <value>${project.version}</value>
                        </replacement>
                    </replacements>
                </configuration>
            </plugin>

When this technique is used within plugin-toolkit, it's equivalent to loading the information from a resource file. This is because compile-time and runtime version of the plugin toolkit is always the same (unless someone tinkers with with plugin internal classpath, but that's not a problem we're trying to solve here).

It would be useful if we wanted the SPI to publish it's compile-time version as a compile-time constant so that it gets inlined. However, I rejected this idea because I didn't find relying on compile-time constants implicit inlining to be a very readable "API".

@findepi
Copy link
Member Author

findepi commented Apr 20, 2022

Thank you @leveyja @losipiuk for your thorough review!

@findepi findepi merged commit e290db1 into trinodb:master Apr 20, 2022
@findepi findepi deleted the findepi/version-spi-3 branch April 20, 2022 07:44
@github-actions github-actions bot added this to the 378 milestone Apr 20, 2022
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.

3 participants