Skip to content

Simplify synonyms YAML tests after auto expand replicas changed #120700

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

Conversation

carlosdelest
Copy link
Member

@carlosdelest carlosdelest commented Jan 23, 2025

Follow up to #115078

After changing auto-expand replicas to 0-1 for synonyms system index, we can now simplify the YAML tests.

Now we can wait for green on the synonyms index, knowing that:

  • For bwc tests mixed cluster tests, there will be 2 updated nodes and 2 non-updated nodes.

    • If a updated node receives the put synonyms request, it will use auto-expand-replicas 0-1. That means that at least the other updated node will be able to create a replica (as non-updated nodes will be in a previous version), so the index will be green.
    • If a non-updated node receives the put synonyms request, it will use auto-expand-replicas 0-all. But as the updated nodes will be able to host replicas (index was created in a previous version), all nodes will be able to hold replicas, so the index will be green.
  • For serverless, we will wait until the index is green with no timeout, so the tests will wait for a search replica to be available before proceeding.

@carlosdelest carlosdelest added >test Issues or PRs that are addressing/adding tests :Search Relevance/Analysis How text is split into tokens auto-backport Automatically create backport pull requests when merged Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.0.0 v8.18.0 labels Jan 23, 2025
@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jan 23, 2025
@carlosdelest carlosdelest marked this pull request as ready for review January 23, 2025 15:02
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

🤔 This might work. Mixed cluster behavior is still dubious to me.

@@ -14,10 +14,8 @@ setup:
# This is to ensure that all index shards (write and read) are available. In serverless this can take some time.
- do:
cluster.health:
index: .synonyms-2
timeout: 2s
index: .synonyms
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but for my educaiton: what is .synynoms-2? Why we added it?

Copy link
Member Author

Choose a reason for hiding this comment

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

.synonyms-2 is the current version of the index - we changed the index format before releasing it, so it has a -2 suffix to it.

.synonyms is the alias for the system indices. I think it's more correct to refer to the alias instead of the specific index, so in case it changes we don't have to change all the YAML tests.

So, it's unrelated to this change, but I thought we could do some winter cleaning here 🙂

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

Thanks, Carlos, makes sense

@carlosdelest carlosdelest merged commit 8fc5a50 into elastic:main Jan 24, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

carlosdelest added a commit to carlosdelest/elasticsearch that referenced this pull request Jan 24, 2025
@carlosdelest
Copy link
Member Author

BwC tests are failing in the backport to 8.x . So it seems that the following is not true:

If a updated node receives the put synonyms request, it will use auto-expand-replicas 0-1. That means that at least the other updated node will be able to create a replica (as non-updated nodes will be in a previous version), so the index will be green.

I'm confused about why an upgraded node (with the auto-expand replicas set to 0-1) would create an index with auto-expand replicas set to 0-all. 🤔

I'll watch test failures on main. That seems to be working as bwc tests on 8.18 already include the auto expand replica change.

Worst case scenario, we don't backport this test simplification to 8.x, and keep it in main. And eventually, we'll stop supporting 8.17 and 8.16 so we'll forget about this. I won't, but others will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Search Relevance/Analysis How text is split into tokens serverless-linked Added by automation, don't add manually Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch >test Issues or PRs that are addressing/adding tests v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants