Skip to content
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

Fix synonyms CI tests timeout #114641

Conversation

carlosdelest
Copy link
Member

Closes #114444
Closes #114443
Closes #114432

(Hopefully) 🤞

Using an alias was making the test flaky. Changed that back to the original version in #114400, and also added this to the rest of the synonyms tests that could be affected by this issue.

@carlosdelest carlosdelest requested a review from kingherc October 11, 2024 16:15
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.0.0 labels Oct 11, 2024
@carlosdelest carlosdelest added :Search Relevance/Analysis How text is split into tokens >test-failure Triaged test failures from CI Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch and removed needs:triage Requires assignment of a team area label Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Oct 11, 2024
@elasticsearchmachine elasticsearchmachine added the needs:risk Requires assignment of a risk label (low, medium, blocker) label Oct 11, 2024
@elasticsearchmachine
Copy link
Collaborator

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

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.

@carlosdelest Thanks for fixing. LGTM!

Copy link
Contributor

@kingherc kingherc left a comment

Choose a reason for hiding this comment

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

LGTM! If there are still failures, maybe increasing the timeout might help. Or, ignoring the timeout altogether may be helpful:

  - do:
      cluster.health:
        index: .synonyms-2
        timeout: 1m
        wait_for_status: green
        ignore: 408

It will let requests go through then, but it's still annoying that the health might not be green (I'd expect ideally tests to use the synonyms with green health rather than yellow).

@carlosdelest carlosdelest requested review from a team as code owners October 15, 2024 11:32
@@ -5,7 +5,7 @@ steps:
steps:
- label: "{{matrix.image}} / docker / packaging-tests-unix"
key: "packaging-tests-unix-docker"
command: ./.ci/scripts/packaging-test.sh destructiveDistroTest.docker
command: ./.ci/scripts/packaging-test.sh destructiveDistroTest.docker-cloud-ess
Copy link
Member

Choose a reason for hiding this comment

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

bad merge?

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 screwed up - will get back to this

@carlosdelest carlosdelest force-pushed the carlosdelest/fix-synonyms-ci-tests-timeout branch 2 times, most recently from 8aa2910 to d4bd80a Compare October 16, 2024 17:50
@carlosdelest
Copy link
Member Author

Reopening to test this suggestion

@carlosdelest carlosdelest reopened this Oct 21, 2024
@carlosdelest
Copy link
Member Author

@elasticmachine update branch

@carlosdelest carlosdelest marked this pull request as ready for review October 22, 2024 09:16
…ts-timeout' into carlosdelest/fix-synonyms-ci-tests-timeout

# Conflicts:
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/10_synonyms_put.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/110_synonyms_invalid.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/20_synonyms_get.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/30_synonyms_delete.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/40_synonyms_sets_get.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/50_synonym_rule_put.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/60_synonym_rule_get.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/70_synonym_rule_delete.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/80_synonyms_from_index.yml
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/90_synonyms_reloading_for_synset.yml
@carlosdelest carlosdelest force-pushed the carlosdelest/fix-synonyms-ci-tests-timeout branch from 5edd935 to bc07059 Compare October 22, 2024 10:28
@@ -58,6 +62,10 @@ setup:

- match: { result: "created" }

- do:
cluster.health:
wait_for_no_initializing_shards: true
Copy link
Member

Choose a reason for hiding this comment

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

😍

Does this work? Seems likely?

@carlosdelest
Copy link
Member Author

Using the changes commented by @kingherc makes this pass. Serverless checks also pass (see PR 3015 in serverless).

Copy link
Contributor

@kingherc kingherc left a comment

Choose a reason for hiding this comment

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

Awesome that it worked! Please note that due to #115019 we had to revert the fast refresh behavioral change, but I think it still makes sense to merge this so that the tests are ready for when we re-incorporate the change in the near future.

Feel free to get a search approval as well before merging.

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.

:shipit: HUZZAH!

@carlosdelest carlosdelest merged commit ba7d095 into elastic:main Oct 23, 2024
16 checks passed
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Oct 23, 2024
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:risk Requires assignment of a risk label (low, medium, blocker) :Search Relevance/Analysis How text is split into tokens Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch >test-failure Triaged test failures from CI v9.0.0
Projects
None yet
6 participants