Skip to content

feat(hunspell): Add reload_cached_resources in TokenFilterFactory to support dictionary hot-reload - #21559

Merged
cwperks merged 2 commits into
opensearch-project:mainfrom
shayush622:feat/hunspell-invalidate-cache
May 15, 2026
Merged

feat(hunspell): Add reload_cached_resources in TokenFilterFactory to support dictionary hot-reload#21559
cwperks merged 2 commits into
opensearch-project:mainfrom
shayush622:feat/hunspell-invalidate-cache

Conversation

@shayush622

@shayush622 shayush622 commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Adds cache reload support to enable hot-reload of hunspell dictionaries via the existing _refresh_search_analyzers API without requiring a node restart.

Problem: When hunspell dictionary files are updated on disk, calling _refresh_search_analyzers rebuilds the analyzer factories but reuses the cached Dictionary object from HunspellService's node-level cache. The
only way to pick up new dictionary content was to restart the node.

Solution: Add a boolean reloadCachedResources parameter to MapperService.reloadSearchAnalyzers(). When true, the method walks each ReloadableCustomAnalyzer's token filters and calls reloadCachedResources() — a
new default method on TokenFilterFactory. HunspellTokenFilterFactory overrides it to atomically reload its dictionary from disk into the cache (no eviction window). The rebuilt factory then picks up the fresh
dictionary.

Changes:

  • TokenFilterFactory.java — adds default void reloadCachedResources() {} interface method
  • MapperService.java — new overload reloadSearchAnalyzers(registry, boolean); existing single-arg method delegates with false (backward compatible). Uses pattern matching and Arrays.stream().forEach() for the
    reload walk.
  • HunspellTokenFilterFactory.java — stores hunspellService/refPath/locale fields (assigned after validation); adds updateable flag support (AnalysisMode.SEARCH_TIME); overrides reloadCachedResources()
  • HunspellService.java — adds reloadDictionaryFromRefPath(refPath, locale) and reloadDictionary(locale) methods that atomically load fresh from disk and replace the cached entry

Related

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here
(https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

Copilot AI review requested due to automatic review settings May 8, 2026 07:32
@github-actions

github-actions Bot commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 7e3bf82)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

In reloadCachedResources(), if both refPath and locale are null, the method silently does nothing. This can occur if the factory was constructed with neither parameter set (though the constructor should prevent this). If the constructor logic changes or is bypassed, reload requests would fail silently instead of reporting the misconfiguration.

 
public void reloadCachedResources() {
    if (refPath != null) {
        hunspellService.reloadDictionaryFromRefPath(refPath, locale);
    } else if (locale != null) {
        hunspellService.reloadDictionary(locale);
    }
}
Possible Issue

The method iterates all analyzers and calls reloadCachedResources() on token filters before rebuilding factories. If an analyzer is not a ReloadableCustomAnalyzer, it is skipped in the reload loop (line 865) but still processed in the rebuild loop (line 878). This mismatch means non-reloadable analyzers will attempt reload without prior cache refresh, potentially causing inconsistent state if they somehow depend on cached resources.

 
if (reloadCachedResources) {
    // Reload cached resources BEFORE building new factories so they pick up fresh data
    for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
        if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
            Arrays.stream(analyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
        }
    }
}

// Build new factories — they will load fresh dictionaries from disk
final Map<String, TokenizerFactory> tokenizerFactories = registry.buildTokenizerFactories(indexSettings);
final Map<String, CharFilterFactory> charFilterFactories = registry.buildCharFilterFactories(indexSettings);
final Map<String, TokenFilterFactory> tokenFilterFactories = registry.buildTokenFilterFactories(indexSettings);
final Map<String, Settings> settings = indexSettings.getSettings().getGroups("index.analysis.analyzer");
final List<String> reloadedAnalyzers = new ArrayList<>();
for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
    if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
        String analyzerName = namedAnalyzer.name();
        Settings analyzerSettings = settings.get(analyzerName);
        analyzer.reload(analyzerName, analyzerSettings, tokenizerFactories, charFilterFactories, tokenFilterFactories);
        reloadedAnalyzers.add(analyzerName);

@github-actions

github-actions Bot commented May 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 7e3bf82

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Update dictionary field after reload

After reloading the dictionary in the cache, the factory's dictionary field still
references the old instance. The factory should update its dictionary field to
reference the newly loaded dictionary, otherwise token filters created after reload
will still use the stale dictionary.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [153-159]

 @Override
 public void reloadCachedResources() {
+    Dictionary freshDictionary;
     if (refPath != null) {
-        hunspellService.reloadDictionaryFromRefPath(refPath, locale);
+        freshDictionary = hunspellService.reloadDictionaryFromRefPath(refPath, locale);
     } else if (locale != null) {
-        hunspellService.reloadDictionary(locale);
+        freshDictionary = hunspellService.reloadDictionary(locale);
+    } else {
+        return;
     }
+    // Update the factory's dictionary reference
+    this.dictionary = freshDictionary;
 }
Suggestion importance[1-10]: 10

__

Why: Critical bug: reloadCachedResources() updates the cache but doesn't update the factory's dictionary field, so create() will continue using the stale dictionary instance. The factory must reassign this.dictionary to the freshly loaded dictionary.

High
Remove final modifier for mutability

The dictionary field is declared as final, preventing it from being updated during
cache reload. Remove the final modifier to allow the field to be reassigned when
reloadCachedResources() is called, ensuring the factory uses the freshly loaded
dictionary.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [74]

-private final Dictionary dictionary;
+private Dictionary dictionary;
Suggestion importance[1-10]: 10

__

Why: The dictionary field is declared final but must be reassigned in reloadCachedResources() for hot-reload to work. Removing final is essential to enable the dictionary reference to be updated with fresh instances.

High
Reload timing issue with cache

The code reloads cached resources from old analyzer instances before building new
factories. This means the new factories will still load from the old cache. Instead,
reload cached resources after building new factories but before calling
analyzer.reload(), so the reload operation picks up the fresh dictionaries.

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-869]

+// Build new factories — they will load fresh dictionaries from disk
+final Map<String, TokenizerFactory> tokenizerFactories = registry.buildTokenizerFactories(indexSettings);
+final Map<String, CharFilterFactory> charFilterFactories = registry.buildCharFilterFactories(indexSettings);
+final Map<String, TokenFilterFactory> tokenFilterFactories = registry.buildTokenFilterFactories(indexSettings);
+
 if (reloadCachedResources) {
-    // Reload cached resources BEFORE building new factories so they pick up fresh data
-    for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
-        if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
-            Arrays.stream(analyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
-        }
-    }
+    // Reload cached resources from new factories before analyzer reload
+    tokenFilterFactories.values().forEach(TokenFilterFactory::reloadCachedResources);
 }
Suggestion importance[1-10]: 9

__

Why: Critical logic error: reloading cached resources from old analyzer instances before building new factories means the new factories will still load stale dictionaries from cache. The reload should happen on the new tokenFilterFactories after they're built but before analyzer.reload() is called.

High

Previous suggestions

Suggestions up to commit c08d099
CategorySuggestion                                                                                                                                    Impact
General
Verify analyzer type coverage

The reload loop only processes ReloadableCustomAnalyzer instances, potentially
missing other analyzer types that contain token filters with cached resources.
Consider iterating all analyzers and checking if they expose token filters, not just
reloadable custom analyzers.

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-869]

 if (reloadCachedResources) {
     // Reload cached resources BEFORE building new factories so they pick up fresh data
     for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
-        if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
-            Arrays.stream(analyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
+        Analyzer analyzer = namedAnalyzer.analyzer();
+        if (analyzer instanceof ReloadableCustomAnalyzer reloadableAnalyzer) {
+            Arrays.stream(reloadableAnalyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
         }
     }
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion asks to verify if other analyzer types should be considered, which is a valid concern. However, the improved_code is nearly identical to existing_code (just uses pattern variable), making this more of a verification request than a concrete improvement.

Low
Log reload failure with context

If loadDictionaryFromRefPath fails, the old dictionary is left in the cache but an
exception is thrown. This creates inconsistency where the cache still has stale data
but the caller thinks reload failed. Consider preserving the old dictionary on
failure or documenting this behavior clearly.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [401-424]

 public Dictionary reloadDictionaryFromRefPath(String refPath, String locale) {
     if (Strings.isNullOrEmpty(refPath)) {
         throw new IllegalArgumentException("refPath cannot be null or empty");
     }
     if (Strings.isNullOrEmpty(locale)) {
         throw new IllegalArgumentException("locale cannot be null or empty");
     }
 
     String cacheKey = buildRefPathCacheKey(refPath, locale);
 
     final Dictionary freshDictionary;
     try {
         freshDictionary = loadDictionaryFromRefPath(refPath, locale);
     } catch (Exception e) {
+        logger.error("Failed to reload hunspell dictionary for ref_path [{}] locale [{}], keeping old dictionary", refPath, locale, e);
         throw new IllegalStateException(
             String.format(Locale.ROOT, "Failed to reload hunspell dictionary for ref_path [%s] locale [%s]", refPath, locale),
             e
         );
     }
 
     dictionaries.put(cacheKey, freshDictionary);
     logger.debug("Reloaded hunspell dictionary cache for key [{}]", cacheKey);
     return freshDictionary;
 }
Suggestion importance[1-10]: 4

__

Why: Adding error logging before throwing an exception provides better observability. However, the suggestion's concern about "keeping old dictionary" is misleading since the cache is not modified on failure, and the improvement only adds logging without changing behavior.

Low
Handle null refPath and locale

The method doesn't handle the case where both refPath and locale are null, which
could occur if the constructor threw an exception before setting these fields. Add
validation or a fallback to prevent silent failures during reload operations.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [153-159]

 @Override
 public void reloadCachedResources() {
     if (refPath != null) {
         hunspellService.reloadDictionaryFromRefPath(refPath, locale);
     } else if (locale != null) {
         hunspellService.reloadDictionary(locale);
+    } else {
+        logger.warn("Cannot reload hunspell dictionary: both refPath and locale are null");
     }
 }
Suggestion importance[1-10]: 2

__

Why: The constructor ensures that either refPath or locale is set (throws exception otherwise), so both cannot be null at runtime. The suggestion addresses an impossible scenario and adds unnecessary defensive code.

Low
Suggestions up to commit e9bd98c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard reload with updateable check

The reloadCachedResources() method should only reload when updateable is enabled.
Currently, it reloads regardless of the updateable flag, which could cause
unexpected behavior for non-updateable filters. Add a guard to check analysisMode ==
AnalysisMode.SEARCH_TIME before reloading.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [153-159]

 @Override
 public void reloadCachedResources() {
+    if (analysisMode != AnalysisMode.SEARCH_TIME) {
+        return;
+    }
     if (refPath != null) {
         hunspellService.reloadDictionaryFromRefPath(refPath, locale);
     } else if (locale != null) {
         hunspellService.reloadDictionary(locale);
     }
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that reloadCachedResources() should only reload when updateable is enabled (analysisMode == SEARCH_TIME). Without this guard, non-updateable filters would reload unnecessarily, which contradicts the design intent of the updateable flag.

Medium
General
Log reload failures before throwing

If loadDictionaryFromRefPath() fails, the old dictionary remains in the cache, but
the method throws an exception. This could leave the system in an inconsistent state
where the reload failed but the cache wasn't updated. Consider logging the failure
and returning the existing dictionary instead of throwing, or ensure the caller
handles the exception properly.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [401-424]

 public Dictionary reloadDictionaryFromRefPath(String refPath, String locale) {
     if (Strings.isNullOrEmpty(refPath)) {
         throw new IllegalArgumentException("refPath cannot be null or empty");
     }
     if (Strings.isNullOrEmpty(locale)) {
         throw new IllegalArgumentException("locale cannot be null or empty");
     }
 
     String cacheKey = buildRefPathCacheKey(refPath, locale);
 
     final Dictionary freshDictionary;
     try {
         freshDictionary = loadDictionaryFromRefPath(refPath, locale);
     } catch (Exception e) {
+        logger.error("Failed to reload hunspell dictionary for ref_path [{}] locale [{}], keeping existing cache", refPath, locale, e);
         throw new IllegalStateException(
             String.format(Locale.ROOT, "Failed to reload hunspell dictionary for ref_path [%s] locale [%s]", refPath, locale),
             e
         );
     }
 
     dictionaries.put(cacheKey, freshDictionary);
     logger.debug("Reloaded hunspell dictionary cache for key [{}]", cacheKey);
     return freshDictionary;
 }
Suggestion importance[1-10]: 5

__

Why: Adding error logging before throwing an exception improves debuggability. However, the improved_code is nearly identical to existing_code (only adds a logger.error call), and the suggestion's concern about "inconsistent state" is overstated since the exception prevents cache corruption. The improvement is minor.

Low
Reload filters in non-reloadable analyzers

The reload logic only processes ReloadableCustomAnalyzer instances, but
non-reloadable analyzers with updateable filters will be skipped. This means their
cached resources won't be reloaded even when requested. Consider iterating all
analyzers to ensure updateable filters are reloaded regardless of analyzer type.

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-869]

 if (reloadCachedResources) {
     // Reload cached resources BEFORE building new factories so they pick up fresh data
     for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
-        if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
-            Arrays.stream(analyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
+        Analyzer analyzer = namedAnalyzer.analyzer();
+        if (analyzer instanceof ReloadableCustomAnalyzer reloadableAnalyzer) {
+            Arrays.stream(reloadableAnalyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
+        } else if (analyzer instanceof CustomAnalyzer customAnalyzer) {
+            Arrays.stream(customAnalyzer.getComponents().getTokenFilters()).forEach(TokenFilterFactory::reloadCachedResources);
         }
     }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion proposes handling CustomAnalyzer in addition to ReloadableCustomAnalyzer, but the PR code already correctly handles only ReloadableCustomAnalyzer instances. The improved_code references CustomAnalyzer which may not exist in the codebase, and the suggestion doesn't provide strong justification for why non-reloadable analyzers need this treatment.

Low
Suggestions up to commit c6d6c62
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle reload failures gracefully

The cache invalidation loop calls reloadCachedResources() on all token filters, but
this happens before building new factories. If a filter fails during reload, the old
factories remain in use but the cache is partially invalidated, creating an
inconsistent state. Consider wrapping the reload calls in try-catch to handle
failures gracefully and log errors without breaking the entire reload process.

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-871]

 if (invalidateCache) {
     // Invalidate caches BEFORE building new factories so they load fresh from disk
     for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
         if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
             for (TokenFilterFactory filter : analyzer.getComponents().getTokenFilters()) {
-                filter.reloadCachedResources();
+                try {
+                    filter.reloadCachedResources();
+                } catch (Exception e) {
+                    logger.warn("Failed to reload cached resources for filter in analyzer [{}]", namedAnalyzer.name(), e);
+                }
             }
         }
     }
 }
Suggestion importance[1-10]: 7

__

Why: Adding error handling prevents partial cache invalidation from breaking the entire reload process. However, the suggestion could be improved by considering whether to continue or abort the reload after failures, and whether the cache state should be rolled back.

Medium
General
Add defensive state validation

The method silently does nothing if both refPath and locale are null, which should
never happen based on constructor validation. However, if this state occurs due to
future code changes, it could lead to silent failures during cache invalidation.
Consider adding a defensive check or assertion to detect this unexpected state.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [153-159]

 @Override
 public void reloadCachedResources() {
     if (refPath != null) {
         hunspellService.reloadDictionaryFromRefPath(refPath, locale);
     } else if (locale != null) {
         hunspellService.reloadDictionary(locale);
+    } else {
+        throw new IllegalStateException("Both refPath and locale are null - invalid factory state");
     }
 }
Suggestion importance[1-10]: 5

__

Why: While the constructor validates that at least one of refPath or locale is non-null, adding an explicit check in reloadCachedResources() improves defensive programming. However, this scenario should be impossible given the constructor logic, making this a low-priority improvement.

Low
Log reload failures explicitly

If loadDictionaryFromRefPath() throws an exception, the method propagates it but
leaves the cache in an inconsistent state. The old dictionary entry remains in the
cache, but subsequent calls may fail. Consider preserving the old dictionary on
reload failure to maintain service availability, or document that the cache entry is
intentionally left stale.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [401-424]

 public Dictionary reloadDictionaryFromRefPath(String refPath, String locale) {
     if (Strings.isNullOrEmpty(refPath)) {
         throw new IllegalArgumentException("refPath cannot be null or empty");
     }
     if (Strings.isNullOrEmpty(locale)) {
         throw new IllegalArgumentException("locale cannot be null or empty");
     }
 
     String cacheKey = buildRefPathCacheKey(refPath, locale);
 
     final Dictionary freshDictionary;
     try {
         freshDictionary = loadDictionaryFromRefPath(refPath, locale);
     } catch (Exception e) {
+        logger.error("Failed to reload hunspell dictionary for ref_path [{}] locale [{}], keeping old entry", refPath, locale, e);
         throw new IllegalStateException(
             String.format(Locale.ROOT, "Failed to reload hunspell dictionary for ref_path [%s] locale [%s]", refPath, locale),
             e
         );
     }
 
     dictionaries.put(cacheKey, freshDictionary);
     logger.debug("Reloaded hunspell dictionary cache for key [{}]", cacheKey);
     return freshDictionary;
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion adds logging before throwing the exception, which provides minimal value since the exception already contains the error context. The comment about "keeping old entry" is misleading since the old entry is preserved by not updating the cache when an exception occurs.

Low
Suggestions up to commit 353a7a0
CategorySuggestion                                                                                                                                    Impact
General
Handle exceptions during cache reload

The cache invalidation loop calls reloadCachedResources() on all token filters, but
doesn't handle potential exceptions. If any filter's reload fails, it could prevent
subsequent filters from being reloaded. Wrap the reload call in a try-catch to log
errors and continue processing remaining filters.

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-871]

 if (invalidateCache) {
     // Invalidate caches BEFORE building new factories so they load fresh from disk
     for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
         if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer analyzer) {
             for (TokenFilterFactory filter : analyzer.getComponents().getTokenFilters()) {
-                filter.reloadCachedResources();
+                try {
+                    filter.reloadCachedResources();
+                } catch (Exception e) {
+                    logger.warn("Failed to reload cached resources for filter in analyzer [{}]", namedAnalyzer.name(), e);
+                }
             }
         }
     }
 }
Suggestion importance[1-10]: 7

__

Why: Adding exception handling during cache reload is important to prevent one failing filter from blocking others. However, the suggestion could be improved by considering whether to continue or fail-fast depending on the error severity.

Medium
Log error before throwing exception

If loadDictionaryFromRefPath() fails, the method throws an exception but leaves the
cache in an inconsistent state. The old dictionary entry remains in the cache, but
the method signals failure. Consider keeping the old dictionary on failure or
documenting this behavior to prevent unexpected cache states during reload errors.

server/src/main/java/org/opensearch/indices/analysis/HunspellService.java [401-424]

 public Dictionary reloadDictionaryFromRefPath(String refPath, String locale) {
     if (Strings.isNullOrEmpty(refPath)) {
         throw new IllegalArgumentException("refPath cannot be null or empty");
     }
     if (Strings.isNullOrEmpty(locale)) {
         throw new IllegalArgumentException("locale cannot be null or empty");
     }
 
     String cacheKey = buildRefPathCacheKey(refPath, locale);
 
     final Dictionary freshDictionary;
     try {
         freshDictionary = loadDictionaryFromRefPath(refPath, locale);
     } catch (Exception e) {
+        logger.error("Failed to reload hunspell dictionary for ref_path [{}] locale [{}], keeping old dictionary", refPath, locale, e);
         throw new IllegalStateException(
             String.format(Locale.ROOT, "Failed to reload hunspell dictionary for ref_path [%s] locale [%s]", refPath, locale),
             e
         );
     }
 
     dictionaries.put(cacheKey, freshDictionary);
     logger.debug("Reloaded hunspell dictionary cache for key [{}]", cacheKey);
     return freshDictionary;
 }
Suggestion importance[1-10]: 4

__

Why: Adding a log statement before throwing an exception provides better observability, but the improved_code is nearly identical to existing_code except for the log line. The concern about cache inconsistency is valid but not addressed by the suggested change.

Low
Possible issue
Add null check for hunspellService

The method doesn't verify that hunspellService is non-null before calling reload
methods. If hunspellService is null (which could happen during initialization or in
edge cases), this will throw a NullPointerException. Add a null check for
hunspellService before attempting to reload.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [153-159]

 @Override
 public void reloadCachedResources() {
+    if (hunspellService == null) {
+        return;
+    }
     if (refPath != null) {
         hunspellService.reloadDictionaryFromRefPath(refPath, locale);
     } else if (locale != null) {
         hunspellService.reloadDictionary(locale);
     }
 }
Suggestion importance[1-10]: 3

__

Why: While adding a null check for hunspellService is defensive programming, the field is initialized in the constructor and should never be null during normal operation. The suggestion is technically correct but addresses an unlikely edge case.

Low
Suggestions up to commit 1ca69e4
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard cache invalidation with updateable flag

The method evicts cache entries regardless of whether updateable was set to true.
This could cause unintended cache invalidation for non-updateable analyzers. Add a
guard to only invalidate when the filter was configured as updateable.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [154-160]

 @Override
 public void invalidateCache() {
+    if (!updateable) {
+        return;
+    }
     if (refPath != null) {
         hunspellService.invalidateDictionary(HunspellService.buildRefPathCacheKey(refPath, locale));
     } else if (locale != null) {
         hunspellService.invalidateDictionary(locale);
     }
 }
Suggestion importance[1-10]: 8

__

Why: This is a valid concern about unintended cache invalidation. The invalidateCache() method currently evicts cache entries regardless of the updateable setting, which could affect non-updateable analyzers. Adding a guard ensures that only filters configured as updateable have their caches invalidated, preventing potential issues with analyzers that shouldn't be reloaded.

Medium
Store updateable flag for consistency

The updateable flag is read but never stored as a field. If invalidateCache() is
called when updateable=false, it will still evict the dictionary from cache, which
is inconsistent. Store the flag and guard invalidateCache() to only evict when
updateable=true.

server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java [89-91]

 // Check for updateable flag
 boolean updateable = settings.getAsBoolean("updateable", false);
 this.analysisMode = updateable ? AnalysisMode.SEARCH_TIME : AnalysisMode.ALL;
+this.updateable = updateable;
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the updateable flag is not stored as a field, which could lead to inconsistent behavior in invalidateCache(). However, the current implementation doesn't show invalidateCache() being guarded by this flag, so the impact depends on whether such a guard is intended. Storing the flag improves code clarity and enables future consistency checks.

Medium
General
Check analysis mode before invalidating cache

The invalidation loop calls invalidateCache() on all token filters without checking
if they support cache invalidation or are configured as updateable. This could
trigger unintended side effects. Consider adding a check for
AnalysisMode.SEARCH_TIME before calling invalidateCache().

server/src/main/java/org/opensearch/index/mapper/MapperService.java [862-872]

 if (invalidateCache) {
     // Invalidate caches BEFORE building new factories so they load fresh from disk
     for (NamedAnalyzer namedAnalyzer : indexAnalyzers.getAnalyzers().values()) {
         if (namedAnalyzer.analyzer() instanceof ReloadableCustomAnalyzer) {
             ReloadableCustomAnalyzer analyzer = (ReloadableCustomAnalyzer) namedAnalyzer.analyzer();
             for (TokenFilterFactory filter : analyzer.getComponents().getTokenFilters()) {
-                filter.invalidateCache();
+                if (filter.getAnalysisMode() == AnalysisMode.SEARCH_TIME) {
+                    filter.invalidateCache();
+                }
             }
         }
     }
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion adds a check for AnalysisMode.SEARCH_TIME before calling invalidateCache(), which provides an additional safety layer. However, this is somewhat redundant if suggestion 2 is implemented (guarding within invalidateCache() itself). The check is reasonable but may be unnecessary defensive programming if the filter's own invalidateCache() method handles the guard internally.

Low

Copilot AI reviewed May 8, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an opt-in Hunspell dictionary cache invalidation path so that updated dictionary files on disk can be picked up during search analyzer reloads without restarting nodes.

Changes:

  • Add reloadSearchAnalyzers(registry, boolean invalidateCache) to optionally invalidate Hunspell caches before rebuilding reloadable analyzers.
  • Add HunspellService.invalidateDictionary(cacheKey) and HunspellTokenFilterFactory.invalidateCache() to support targeted eviction (traditional locale and ref_path keys).
  • Add tests covering cache eviction and updateable/AnalysisMode behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
server/src/main/java/org/opensearch/index/mapper/MapperService.java Adds an overload to optionally invalidate Hunspell caches before reloading search analyzers.
server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java Adds updateable→AnalysisMode.SEARCH_TIME support and exposes a cache invalidation helper.
server/src/main/java/org/opensearch/indices/analysis/HunspellService.java Adds dictionary cache eviction by cache key.
server/src/test/java/org/opensearch/indices/analyze/HunspellServiceTests.java Adds unit tests for service-level cache invalidation behavior.
server/src/test/java/org/opensearch/index/analysis/HunspellTokenFilterFactoryTests.java Adds tests for updateable mode and invalidation behavior (including a simulated invalidation walk).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java
Comment thread server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java
Comment thread server/src/main/java/org/opensearch/indices/analysis/HunspellService.java Outdated
Comment thread server/src/main/java/org/opensearch/index/mapper/MapperService.java Outdated
Comment thread server/src/test/java/org/opensearch/index/analysis/HunspellTokenFilterFactoryTests.java
@shayush622

shayush622 commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

The RaceCondition indicated by the comment above it theoretically possible but not a real concern in practice, because:

  1. Who else would load the dictionary between invalidate and reload? Only a search request hitting the same analyzer on the same node. But reloadSearchAnalyzers is synchronized — it holds the lock on the
    MapperService instance. While it's running, no new analyzer can be built from this MapperService.
  2. The race would be: Thread A invalidates cache → Thread B does a search, triggers getDictionaryFromRefPath via the old still-active analyzer → caches the old dictionary → Thread A's reload builds a new
    factory that gets the re-cached old dictionary.
    But this can't happen because the old analyzer's dictionary field is already loaded (it's a final field set at construction). Searches using the old analyzer don't call getDictionaryFromRefPath again — they
    use the already-loaded Dictionary object directly. The only code that calls getDictionaryFromRefPath is the HunspellTokenFilterFactory constructor during reload().
  3. What about a concurrent reloadSearchAnalyzers on another index? Different MapperService instance, different lock — but they'd only invalidate their own filter's cache key, not ours.

So the window doesn't exist in practice. The dictionary is only re-fetched from cache during factory construction (inside reload()), which happens immediately after invalidation within the same synchronized
block.

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 4003f88: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

RajatGupta02
RajatGupta02 reviewed May 12, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/index/mapper/MapperService.java Outdated
@shayush622
shayush622 force-pushed the feat/hunspell-invalidate-cache branch from 4003f88 to d901c4f Compare May 13, 2026 05:05
@shayush622
shayush622 requested a review from RajatGupta02 May 13, 2026 05:15
@shayush622
shayush622 force-pushed the feat/hunspell-invalidate-cache branch from d901c4f to dc14993 Compare May 13, 2026 05:18
shwetathareja
shwetathareja reviewed May 14, 2026
View reviewed changes
Comment thread server/src/main/java/org/opensearch/index/analysis/HunspellTokenFilterFactory.java Outdated
Comment thread server/src/main/java/org/opensearch/index/mapper/MapperService.java Outdated
@shayush622
shayush622 force-pushed the feat/hunspell-invalidate-cache branch from dc14993 to 353e1d2 Compare May 14, 2026 10:00
cwperks
cwperks reviewed May 14, 2026
View reviewed changes
Comment thread server/src/test/java/org/opensearch/index/analysis/HunspellTokenFilterFactoryTests.java Outdated
@shayush622
feat(hunspell): add cache invalidation to support dictionary hot-reload
c08d099 
Signed-off-by: shayush622 <ayush5267@gmail.com>
@shayush622
shayush622 force-pushed the feat/hunspell-invalidate-cache branch from e9bd98c to c08d099 Compare May 14, 2026 17:47
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c08d099

@shayush622
shayush622 requested a review from cwperks May 14, 2026 17:48
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for c08d099: SUCCESS

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 7e3bf82

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@shayush622 shayush622 mentioned this pull request May 15, 2026
Merged
3 tasks
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@shayush622 shayush622 changed the title feat(hunspell): Add cache invalidation to support dictionary hot-reload feat(hunspell): Add reload_cached_resources in TokenFilterFactory to support dictionary hot-reload May 15, 2026
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 7e3bf82: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

❕ Gradle check result for 7e3bf82: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@cwperks
cwperks merged commit 4430adc into opensearch-project:main May 15, 2026
46 of 62 checks passed
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request May 17, 2026
@shayush622 @shwetathareja Khishorekumar BS
feat(hunspell): add cache invalidation to support dictionary hot-relo…
efb70dd 
…ad (opensearch-project#21559)

Signed-off-by: shayush622 <ayush5267@gmail.com>
Co-authored-by: shwetathareja <shwetathareja@live.com>
Signed-off-by: Khishorekumar BS <bkhishor@amazon.com>
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request May 27, 2026
Merged
@shayush622 shayush622 mentioned this pull request May 29, 2026
Merged
1 task
This was referenced Jun 17, 2026
Open
Open
Closed
Open
Open
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Jun 29, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@shwetathareja shwetathareja shwetathareja approved these changes
@cwperks cwperks cwperks approved these changes
@andrross andrross Awaiting requested review from andrross andrross is a code owner
@ashking94 ashking94 Awaiting requested review from ashking94 ashking94 is a code owner
@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner
@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner
@gbbafna gbbafna Awaiting requested review from gbbafna gbbafna is a code owner
@jed326 jed326 Awaiting requested review from jed326 jed326 is a code owner
@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner
@mch2 mch2 Awaiting requested review from mch2 mch2 is a code owner
@msfroh msfroh Awaiting requested review from msfroh msfroh is a code owner
@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner
@reta reta Awaiting requested review from reta reta is a code owner
@sachinpkale sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner
@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner
@sohami sohami Awaiting requested review from sohami sohami is a code owner
@RajatGupta02 RajatGupta02 Awaiting requested review from RajatGupta02

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shayush622 @shwetathareja @cwperks @RajatGupta02