Skip to content

Add lucene version compatibility tests #121104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 29, 2025
Merged

Add lucene version compatibility tests #121104

merged 6 commits into from
Jan 29, 2025

Conversation

mark-vieira
Copy link
Contributor

This commit adds compatibility tests that target ES revisions that align with specific Lucene versions. In this case, we are intending to upgrade from Lucene 10.0 to 10.1. Since no on-prem Elasticsearch release exists with 10.0, we need another method to ensure compatibility with Lucene 10.0 indicies.

The work here is a bit hacky since all our compatibility testing infrastructure is centered around versions and we're now effectively doing compatibility tests between two different revisions of Elasticsearch that both report the same version. Ideally this specific testing would be replaced by unit tests, rather that reusing our full cluster restart tests for this purpose.

We'll also want to bump the commit referenced in the CI pipelines here to align with the last commit using Lucene 10.0.

@mark-vieira mark-vieira added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure labels Jan 28, 2025
@mark-vieira mark-vieira requested a review from a team as a code owner January 28, 2025 21:57
@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Jan 28, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

LGTM

ES_VERSION:
- "9.0.0"
ES_COMMIT:
- "b2cc9d9b8f00ee621f93ddca07ea9c671aab1578" # update to match last commit before lucene bump
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we wanna coordinate on this commit hash, or just go with what is here and update it after the Lucene 10.1.0 upgrade has been merged?

Copy link
Member

Choose a reason for hiding this comment

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

Tying testing to a specific commit is ok, yet I wonder if it would be possible in the future to tie to index version changes. I think that besides codec changes, we are totally lacking test coverage for index version changes that we make. And a codec change implies an index version change.

Copy link
Contributor Author

@mark-vieira mark-vieira Jan 29, 2025

Choose a reason for hiding this comment

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

We have to know what hash to checkout. So if we do based on index version we still have to have some mapping of version -> commit somewhere.

@@ -102,6 +102,29 @@ private void registerInternalDistributionResolutions(List<DistributionResolution
}
return null;
}));

// Distribution resolution for "override" versions. This allows for building from source for any version, including the current
// version of existing released versions from a commit form the main branch. This is done by passing certain system properties, ex:
Copy link
Contributor

Choose a reason for hiding this comment

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

minor, typo "... form the main branch"

String versionProperty = System.getProperty("tests.bwc.main.version");
// We use this phony version as a placeholder for the real version
if (distribution.getVersion().equals("0.0.0")) {
BwcVersions.UnreleasedVersionInfo unreleasedVersionInfo = new BwcVersions.UnreleasedVersionInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to assert that versionProperty is non-null here? Or maybe it's already implied by the following fromString ?

@@ -20,6 +21,13 @@ buildParams.bwcVersions.withIndexCompatible { bwcVersion, baseName ->
}
}

tasks.register("luceneBwcTest", StandaloneRestIntegTestTask) {
// We use a phony version here as the real version is provided via `tests.bwc.main.version` system property
usesBwcDistribution(Version.fromString("0.0.0"))
Copy link
Contributor

Choose a reason for hiding this comment

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

ha! this is a cute trick. Nice!

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jan 29, 2025
@mark-vieira mark-vieira merged commit 61dc931 into main Jan 29, 2025
17 checks passed
@mark-vieira mark-vieira deleted the lucene-compat-testing branch January 29, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure serverless-linked Added by automation, don't add manually Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants