Non existing synonyms sets do not fail shard recovery #125659

carlosdelest · 2025-03-26T12:13:56Z

When a synonyms set does not exist, it doesn't fail shard recovery, but logs an INFO message and continues.

Using lenient: false on the synonym analyzer restores the previous behaviour, in which the shards cannot be allocated for the index.

carlosdelest · 2025-03-26T12:16:09Z

qa/mixed-cluster/build.gradle

 excludeList.add("cat.shards/10_basic/Help")

-// Can't work until auto-expand replicas is 0-1 for synonyms index
-excludeList.add("synonyms/90_synonyms_reloading_for_synset/Reload analyzers for specific synonym set")


This is unrelated to this change, but a good opportunity to enable this test again

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/110_synonyms_invalid.yml

mayya-sharipova · 2025-03-26T13:17:48Z

server/src/main/java/org/elasticsearch/index/analysis/Analysis.java

+            if (ignoreMissing == false) {
+                throw e;
+            }
+            logger.info(


should this be logger.error instead?

I thought about that - but I don't think it's something that prevents usage or indicates a problem on Elasticsearch. I think it's similar to having an invalid synonym rule.

On the other hand, duplicate terms in dictionaries are set as WARN.

I'm changing it to WARN, does that sound good?

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/110_synonyms_invalid.yml

…existing-synonyms' into bugfix/synonyms-error-non-existing-synonyms

carlosdelest · 2025-03-26T14:07:25Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/80_synonyms_from_index.yml

  - match: { hits.total.value: 1 }

---
-"Fail loading synonyms from index if synonyms_set doesn't exist":


I moved this test to 100_synonyms_invalid

elasticsearchmachine · 2025-03-26T14:08:24Z

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

elasticsearchmachine · 2025-03-26T14:08:24Z

Pinging @elastic/search-eng (Team:SearchOrg)

elasticsearchmachine · 2025-03-26T14:08:25Z

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

jimczi

Thanks @carlosdelest, I left some comments

jimczi · 2025-03-26T14:16:11Z

server/src/main/java/org/elasticsearch/index/analysis/Analysis.java

+            if (ignoreMissing == false) {
+                throw e;
+            }
+            logger.warn(


Should this be at the error level?

I also wonder if we should handle all exceptions rather than just the resource not found one?
Maybe we should even retry if that's appropriate (based on the exception)?
I am fine if we do that in a follow up but a comment here would help to understand why we do that.

Should this be at the error level?

I was discussing this with @mayya-sharipova - I think this is not something that prevents ES from working, it's more of a user error. I think WARN is the appropriate level.

I also wonder if we should handle all exceptions rather than just the resource not found one?
That's a good point - Adding that as a log with ERROR level in ecb2f26

Maybe we should even retry if that's appropriate (based on the exception)?

I'm not sure, what errors would benefit from a retry?

Shard allocation already is tried a number of times, so I think we should be covered here. In case it can be fixed after some additional action, reroute can be used to recover the shard.

Shard allocation already is tried a number of times, so I think we should be covered here. In case it can be fixed after some additional action, reroute can be used to recover the shard.

Yep that's fine, let's monitor the exception we catch here to be proactive.

Regarding the catching of exceptions, I am not sure that only ElasticsearchException are thrown by the call to actionGet, should we catch all Exception instead?

I am not sure that only ElasticsearchException are thrown by the call to actionGet, should we catch all Exception instead?

That makes sense, done in b88fdc4

jimczi · 2025-03-26T14:18:03Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/CreateIndexCapabilities.java

        POORLY_FORMATTED_BAD_REQUEST,
-        HUNSPELL_DICT_400
+        HUNSPELL_DICT_400,
+        SYNONYMS_SET_LENIENT_ON_NON_EXISTING


That's just for tests no? Can we separate it and only add it when testing?

Maybe create a test feature?

Ah thanks, I'm too used to caps right now - changed on 636bea9

kderusso

Nice work Carlos!

kderusso · 2025-03-26T14:30:34Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/CreateIndexCapabilities.java

        POORLY_FORMATTED_BAD_REQUEST,
-        HUNSPELL_DICT_400
+        HUNSPELL_DICT_400,
+        SYNONYMS_SET_LENIENT_ON_NON_EXISTING


Maybe create a test feature?

mayya-sharipova

@carlosdelest Thanks for the quick work on this! Makes sense!

jimczi

LGTM

carlosdelest · 2025-03-27T09:20:42Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

…existing-synonyms' into bugfix/synonyms-error-non-existing-synonyms

elasticsearchmachine · 2025-03-27T16:06:13Z

💔 Backport failed

Status	Branch	Result
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts
✅	9.0

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 125659

(cherry picked from commit 968bddc) # Conflicts: # qa/mixed-cluster/build.gradle # rest-api-spec/build.gradle

carlosdelest · 2025-03-27T16:15:15Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	8.18

Questions ?

Please refer to the Backport tool documentation

(cherry picked from commit 968bddc) # Conflicts: # qa/mixed-cluster/build.gradle # rest-api-spec/build.gradle

)

#125796)

…#125795)

(cherry picked from commit 968bddc)

) (cherry picked from commit 968bddc)

(cherry picked from commit 968bddc) # Conflicts: # qa/mixed-cluster/build.gradle # rest-api-spec/build.gradle

#125479) (#127981) * Non existing synonyms sets do not fail shard recovery (#125659) (cherry picked from commit 968bddc) # Conflicts: # qa/mixed-cluster/build.gradle # rest-api-spec/build.gradle * Fix backport merge

carlosdelest added 2 commits March 26, 2025 13:12

Use lenient to ignore non existing synonyms sets

a77c9a4

Removes excluded synonyms set test

88abdff

elasticsearchmachine added the v9.1.0 label Mar 26, 2025

carlosdelest commented Mar 26, 2025

View reviewed changes

[CI] Auto commit changes from spotless

41e66ce

mayya-sharipova reviewed Mar 26, 2025

View reviewed changes

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/110_synonyms_invalid.yml Show resolved Hide resolved

mayya-sharipova reviewed Mar 26, 2025

View reviewed changes

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/synonyms/110_synonyms_invalid.yml Show resolved Hide resolved

carlosdelest added 3 commits March 26, 2025 15:01

Change log level to warn

ad8474f

Removed test that has been moved to 110_synonyms_invalid

5a0149e

Merge remote-tracking branch 'carlosdelest/bugfix/synonyms-error-non-…

1623a0e

…existing-synonyms' into bugfix/synonyms-error-non-existing-synonyms

carlosdelest commented Mar 26, 2025

View reviewed changes

carlosdelest marked this pull request as ready for review March 26, 2025 14:08

carlosdelest requested review from a team and jimczi March 26, 2025 14:08

carlosdelest changed the title ~~Non existing synonyms sets does not fail shard recovery~~ Non existing synonyms sets do not fail shard recovery Mar 26, 2025

jimczi reviewed Mar 26, 2025

View reviewed changes

kderusso approved these changes Mar 26, 2025

View reviewed changes

carlosdelest added 2 commits March 26, 2025 16:37

Use test features instead of capabilities

636bea9

Removed test is skipped from rest compat test

dd3db4d

mayya-sharipova approved these changes Mar 26, 2025

View reviewed changes

carlosdelest requested a review from jimczi March 27, 2025 06:53

carlosdelest and others added 2 commits March 27, 2025 10:06

Catch Exception to ensure we catch'em'all

b88fdc4

Merge branch 'main' into bugfix/synonyms-error-non-existing-synonyms

d78eff2

jimczi approved these changes Mar 27, 2025

View reviewed changes

Merge branch 'main' into bugfix/synonyms-error-non-existing-synonyms

5f813b1

carlosdelest enabled auto-merge (squash) March 27, 2025 12:05

carlosdelest and others added 3 commits March 27, 2025 14:16

Merge branch 'main' into bugfix/synonyms-error-non-existing-synonyms

6ace60b

Catch a mixed cluster warning header for reroute

19955f2

Merge remote-tracking branch 'carlosdelest/bugfix/synonyms-error-non-…

cec345f

…existing-synonyms' into bugfix/synonyms-error-non-existing-synonyms

carlosdelest merged commit 968bddc into elastic:main Mar 27, 2025
16 of 17 checks passed

carlosdelest mentioned this pull request Mar 27, 2025

[9.0] Non existing synonyms sets do not fail shard recovery (#125659) #125794

Merged

elasticsearchmachine added the backport pending label Mar 27, 2025

carlosdelest added a commit to carlosdelest/elasticsearch that referenced this pull request Mar 27, 2025

Non existing synonyms sets do not fail shard recovery (elastic#125659)

6df3b96

carlosdelest mentioned this pull request Mar 27, 2025

[8.x] Non existing synonyms sets do not fail shard recovery (#125659) #125795

Merged

carlosdelest mentioned this pull request Mar 27, 2025

[8.18] Non existing synonyms sets do not fail shard recovery (#125659) #125796

Merged

elasticsearchmachine pushed a commit that referenced this pull request Mar 27, 2025

Non existing synonyms sets do not fail shard recovery (#125659) (#125794

e330e78

)

carlosdelest added a commit that referenced this pull request Mar 28, 2025

[8.18] Non existing synonyms sets do not fail shard recovery (#125659) (

51b715f

#125796)

carlosdelest added a commit that referenced this pull request Mar 28, 2025

[8.x] Non existing synonyms sets do not fail shard recovery (#125659) (…

af4a548

…#125795)

carlosdelest added a commit to carlosdelest/elasticsearch that referenced this pull request Mar 28, 2025

Non existing synonyms sets do not fail shard recovery (elastic#125659)

2abdafc

(cherry picked from commit 968bddc)

breskeby pushed a commit that referenced this pull request Mar 28, 2025

Non existing synonyms sets do not fail shard recovery (#125659) (#125865

7cddef9

) (cherry picked from commit 968bddc)

omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025

Non existing synonyms sets do not fail shard recovery (elastic#125659)

538ca86

carlosdelest mentioned this pull request Apr 2, 2025

Synonyms API: Index gets red status when synonyms set does not exist #125603

Closed

Non existing synonyms sets do not fail shard recovery #125659

Non existing synonyms sets do not fail shard recovery #125659

Uh oh!

Conversation

carlosdelest commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Mar 26, 2025

Uh oh!

elasticsearchmachine commented Mar 26, 2025

Uh oh!

elasticsearchmachine commented Mar 26, 2025

Uh oh!

jimczi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosdelest Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

carlosdelest commented Mar 27, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 27, 2025

💔 Backport failed

Uh oh!

carlosdelest commented Mar 27, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlosdelest commented Mar 26, 2025 •

edited

Loading

jimczi left a comment •

edited

Loading

carlosdelest Mar 27, 2025 •

edited

Loading