Skip to content

Add QA test module for Lucene N-2 version#118363

Merged
tlrx merged 2 commits intoelastic:mainfrom
tlrx:2024/12/10/ES-10274-BWC-test
Dec 11, 2024
Merged

Add QA test module for Lucene N-2 version#118363
tlrx merged 2 commits intoelastic:mainfrom
tlrx:2024/12/10/ES-10274-BWC-test

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Dec 10, 2024

This pull request introduces a new QA project to test Lucene support for reading indices created in version N-2.

The test suite is inspired from the various full-cluster restart suites we already have. It creates a cluster in version N-2 (today 7.17.25), then upgrades the cluster to N-1 (today 8.18.0) and finally upgrades the cluster to the current version (today 9.0), allowing to execute test methods after every upgrade.

The test suite has two variants: one for searchable snapshots and one for snapshot restore. The suites demonstrates that Elasticsearch does not allow reading indices written in version N-2 but we hope to make this feasible. Also, the tests can be used for investigation and debug (launched with --debug-server-jvm).

Relates ES-10274

@tlrx tlrx requested a review from a team as a code owner December 10, 2024 16:41
@tlrx tlrx added >test Issues or PRs that are addressing/adding tests :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v9.0.0 labels Dec 10, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing (obsolete) Meta label for Distributed Indexing team. Obsolete. Please do not use. label Dec 10, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

@javanna
Copy link
Contributor

javanna commented Dec 10, 2024

This reminded me of a similar effort for archive indices in https://github.com/elastic/elasticsearch/pull/116565/files#diff-0127cb27f9161cd76ae90b86b80ee56bad117effcf0bcdcc5941af2a9dc32543 . At this point, I don't expect that PR to get merged.

Thanks a lot for working on this! I see how we could use this as a base to add more tests, similar to what we do for archive indices, but more, for instance test specific mappings compatibility etc.

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

logger.fatal("--> mounting index [{}] as [{}]", index, mountedIndex);

// Mounting the index will fail as Elasticsearch does not support reading N-2 yet
var request = new Request("POST", "/_snapshot/" + repository + "/" + snapshot + "/_mount");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment for later that we should also add a test where we mount the N-2 index on N-1 and then search it on N.

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 added a TODO 👍

import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;

public class ArchiveCompatibilityIT extends AbstractLuceneIndexCompatibilityTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this LuceneCompatibilityIT instead? I think of it as different from archive indices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I also renamed testRestoreArchive to testRestoreIndex

}

public List<Version> getReadOnlyIndexCompatible() {
// Lucene can read indices in version N-2
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should assert that N-2 == 7? We definitely want to utiliize this for the V9 upgrade, but we are not fully committed to necessarily having N-2 support in V10 (not saying we won't either).

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 added an explicit check with a comment in the test suite. I think it achieves the same goal, while avoiding it to be in the build logic.

@tlrx tlrx force-pushed the 2024/12/10/ES-10274-BWC-test branch from a37da6b to 3c2041c Compare December 11, 2024 08:42
@tlrx tlrx requested a review from breskeby December 11, 2024 10:35
@tlrx
Copy link
Member Author

tlrx commented Dec 11, 2024

@breskeby hope you have the time to review the few changes in the BwcVersion class 🙏🏻

@breskeby
Copy link
Contributor

@tlrx @mark-vieira just had touched this file a few days ago and made some changes. I'll delegate to him for having a look at this

@breskeby breskeby requested a review from mark-vieira December 11, 2024 10:52
tasks.named("javaRestTest").configure {
systemProperty("tests.minimum.index.compatible", bwcVersion)
usesBwcDistribution(bwcVersion)
enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

why would you need explicitly enable = true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason it complained that it was disabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes, bwc-test plugin by default disables this task

tasks.named("javaRestTest").configure {
systemProperty("tests.minimum.index.compatible", bwcVersion)
usesBwcDistribution(bwcVersion)
enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes, bwc-test plugin by default disables this task

@tlrx tlrx merged commit e0ad97e into elastic:main Dec 11, 2024
@tlrx tlrx deleted the 2024/12/10/ES-10274-BWC-test branch December 11, 2024 16:03
@tlrx
Copy link
Member Author

tlrx commented Dec 11, 2024

Thanks all!

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

Labels

:Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. Team:Distributed Indexing (obsolete) Meta label for Distributed Indexing team. Obsolete. Please do not use. >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.

5 participants