Verify that no non deprecated SPI is removed

[kokosing]

Verify that no non deprecated SPI is removed

This test verifies that SPI cannot be changed without a deprecation
period.

This partially solves the single version backward compatibility, the
other part is to ensure that engine still calls the deprecated API.

[kokosing]

Partially fixes #11667, the other part is not easily doable via tests. As we would need to have a plugins in previous version. I guess it requires the code review process to make sure that the old API is still used.

[martint]

This will cause tests to fail in offline mode, or in any environment that doesn't have direct access to maven central. In some organizations, all access is performed through a proxy or a local maven repository.

[kokosing]

So it looks like I need to download artifacts via maven, so it should use local maven repository in most of the cases.

[electrum]

You could probably use https://maven.apache.org/plugins/maven-dependency-plugin/copy-mojo.html#dependency-copy

[findepi]

I think it's reasonable to require internet access during tests.
For example, docker-based tests won't pass if it is not possible to download the container images.

cc @nineinchnick

[findepi]

I agree using local maven repo would be better though. Eg it would cover for retries required to cope with central's instability.

This can help

        <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-dependency-plugin</artifactId>
                <executions>
                    <execution>
                        <id>copy</id>
                        <phase>package</phase>
                        <goals>
                            <goal>copy</goal>
                        </goals>
                        <configuration>
                            <skip>false</skip>
                            <artifactItems>
                                <artifactItem>
                                    <groupId>io.trino</groupId>
                                    <artifactId>trino-cli</artifactId>
                                    <version>${dep.trino.version}</version>
                                    <classifier>executable</classifier>
                                    <type>jar</type>
                                    <outputDirectory>${project.build.directory}</outputDirectory>
                                </artifactItem>

[martint]

docker-based tests won't pass if it is not possible to download the container images.

You can always pre-download them and run the tests offline.

[findepi]

be specific which file(s) should be filtered

[findepi]

SPI has more deps than just slice. Why is the Slice special? Document

[kokosing]

Slice is special as loading SPI classes was loading Slice classes. Others were not loaded.

[findepi]

url1 -> inputStream

[findepi]

will we need an scape hatch here?

[findepi]

Are we assuming that Slice version doesn't change?

[kokosing]

It can change, however I assume that loading slice classes for SPI for previous version will still work. Otherwise it sounds like yet another backward incompatible change.

[findepi]

I am not sure i agree, but please capture your reasoning as a code comment

[martint]

This test verifies that SPI cannot be changed without a deprecation
period.

Sometimes that is not possible, and we have to break backward compatibility. This change will entirely prevent that and remove that ability.

[kokosing]

AC

[kokosing]

Slice is special as loading SPI classes was loading Slice classes. Others were not loaded.

[kokosing]

It can change, however I assume that loading slice classes for SPI for previous version will still work. Otherwise it sounds like yet another backward incompatible change.

[kokosing]

Sometimes that is not possible, and we have to break backward compatibility. This change will entirely prevent that and remove that ability.

@martint Notice that this test captured backward incompatibilities that were introduced in current ongoing version. So test does not prevent them, it makes them visible. Now we could potentially add them to release notes.

[findepi]

exclusions (BACKWARD_INCOMPATIBLE_CHANGES) should be verified for being part of previous SPI

eg a test should fail when i add public void foo() to BACKWARD_INCOMPATIBLE_CHANGES, unless such method existed in the previous SPI AND it no longer exists in the new SPI.

[kokosing]

There is a problem with this. If we do this, then surely the very next commit after the release will have red CI due to this (when we had BACKWARD_INCOMPATIBLE_CHANGES in previous release).

I thought that maybe it is ok, to have BACKWARD_INCOMPATIBLE_CHANGES allow for things that got removed few releases ago.

Another thought is to print a warning, so it does not break the build, but it is noticed whenever somebody runs this test.
WDYT?

[findepi]

Alternatively, we could eg have BACKWARD_INCOMPATIBLE_CHANGES as a map version -> changes
and we would just look at the entry for current version.
Of course, the point is not to keep track of all the breaking changes ever, but rather not to fail right after a release.
Then next person adding something new to the list would remove old (obsolete) entries.

[kokosing]

I no longer get a version from the pom to java. So in tests I have no notion of a version. Bringing back the version in the test just for this sounds like an overkill to me.

Maybe I can simply add a comment that:

// when updating  BACKWARD_INCOMPATIBLE_CHANGES please check if you can remove obsolete entries

?

[findepi]

Bringing back the version in the test just for this sounds like an overkill to me.

It's not

BTW i am making Java aware of SPI version in #11774

[kokosing]

@findepi AC

@martint PTAL, I hope I have addressed your concerns.

[martint]

This is generally the case, but might not always be. For instance, if something happens during the release process, we might need to "burn" a release (e.g., https://trino.io/docs/current/release/release-349.html)

[findepi]

we will have to change this code line when doing that. perhaps add an if.

[findepi]

note also that we would have same problem with JDBC and server compatibility testing.
A rare case of a burned release would need to be handled explicitly.

[kokosing]

@martint AC, PTAL

[findepi]

This should be auto-derived IMO. Why hard-coded? it adds maintenance overhead.

Note that when the test fails people will be steered first towards maintaining compatibility with an old version, rather than fix the version number here.

[kokosing]

This is about addressing this comment: #11722 (comment)

Note that when the test fails people will be steered first towards maintaining compatibility with an old version, rather than fix the version number here.

Maybe it is a good thing. Also I can add some comment here to make it more clear what is the goal here. Like We try to be backward compatible with at least last released version. WDYT?

[findepi]

Maybe it is a good thing.

This isn't what was intended and is more than we wanted.

add some comment here to make it more clear

the problem is -- the comment here won't be visible; the test failure will

This is about addressing this comment: #11722 (comment)

i replied there

[findepi]

This is recursive (higher up in class hierarchy), maybe worth commenting about this.

Also, you returning Object-defined methods

public final void java.lang.Object.wait(long,int) throws java.lang.InterruptedException
public final void java.lang.Object.wait() throws java.lang.InterruptedException
public final native void java.lang.Object.wait(long) throws java.lang.InterruptedException
public boolean java.lang.Object.equals(java.lang.Object)
public java.lang.String java.lang.Object.toString()
public native int java.lang.Object.hashCode()
public final native java.lang.Class<?> java.lang.Object.getClass()
public final native void java.lang.Object.notify()
public final native void java.lang.Object.notifyAll()

they will not cause a change (unless last SPI classes is removed), but still maybe worth documenting?

[kokosing]

I changed it to return only declared entities and skip object and enum methods. Thanks.

Previously, used getMethods and getFields as it returns only public things. However it is easier public things than hierarchical ones (or maybe it as same level of complexity...)

[findepi]

This is an example entry which makes adding new entries easier, but this example entry should be removed already (per comment above + 376 is out). Let's make adding new entries easier in some more durable manner, perhaps as a commented-out copy-paste-ready example.

[kokosing]

I am not sure. 375-SNAPSHOT is almost compatible with 375 which is a good thing.

Also since now version is hardcoded, I no longer think that the key ("375") is needed here.

[findepi]

The purpose of key-ing on version was to enforce that backwards incompatible change declaration is not implicitly carried over to the next release.

Let's imagine

• v 100: we have a public method foo
• v 101: we remove the method foo and declare this as a backward incompatible change
• v 102: we reintroduce method foo
• v 103-SNAPSHOT: we remove method foo -- we want the test to fail, as this is a new backward incompatible change; for it to fail, we need to ensure the 101's suppression is not carried forwards implicitly

[kokosing]

AC

[kokosing]

This is about addressing this comment: #11722 (comment)

Note that when the test fails people will be steered first towards maintaining compatibility with an old version, rather than fix the version number here.

Maybe it is a good thing. Also I can add some comment here to make it more clear what is the goal here. Like We try to be backward compatible with at least last released version. WDYT?

[kokosing]

I am not sure. 375-SNAPSHOT is almost compatible with 375 which is a good thing.

Also since now version is hardcoded, I no longer think that the key ("375") is needed here.

[kokosing]

I changed it to return only declared entities and skip object and enum methods. Thanks.

Previously, used getMethods and getFields as it returns only public things. However it is easier public things than hierarchical ones (or maybe it as same level of complexity...)

[kokosing]

AC

[kokosing]

AC

@findepi Thank you for the review! ❤️

[findepi]

@kokosing thank you for your patience with my review comments.
This is an awesome addition!

I am gonna merge this tomorrow, unless anyone has any more comments.

cc @trinodb/maintainers

[kokosing]

thank you for your patience with my review comments.

@findepi This was a pleasure. Always! I appreciate all your comments, they improved this change a lot!

[kokosing]

Thank you!