Skip to content

Fix upgrading indices which use a custom similarity plugin. #26985

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.

Sign up for GitHub

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
merged 4 commits into from
Jan 9, 2018
Merged

Fix upgrading indices which use a custom similarity plugin. #26985

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
824e545
Fix upgrading indices which use a custom similarity plugin.
xabbu42 Oct 11, 2017
976b520
add/combine comments for the fake similarity map
xabbu42 Nov 11, 2017
aa4be49
Fix wrong tabs
xabbu42 Nov 29, 2017
28309da
Merge branch 'master' into review/26985
rjernst Dec 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.index.analysis.NamedAnalyzer;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.similarity.SimilarityService;
import org.elasticsearch.index.similarity.SimilarityProvider;
import org.elasticsearch.indices.mapper.MapperRegistry;

import java.util.AbstractMap;
Expand Down Expand Up @@ -136,7 +137,24 @@ private void checkMappingsCompatibility(IndexMetaData indexMetaData) {
// We cannot instantiate real analysis server at this point because the node might not have
// been started yet. However, we don't really need real analyzers at this stage - so we can fake it
IndexSettings indexSettings = new IndexSettings(indexMetaData, this.settings);
SimilarityService similarityService = new SimilarityService(indexSettings, null, Collections.emptyMap());
final Map<String, SimilarityProvider.Factory> similarityMap = new AbstractMap<String, SimilarityProvider.Factory>() {
Copy link
Member

@rjernst rjernst Oct 12, 2017

Choose a reason for hiding this comment

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

I would add a comment here that having a dummy map works because we assume any similarity providers were known before the upgrade, so allowing any key below is ok.

Copy link
Contributor Author

@xabbu42 xabbu42 Oct 13, 2017

Choose a reason for hiding this comment

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

I did test the patch with a elasticsearch server without the custom similarity installed. The server starts but does not recover the index with an appropriate error message. I did not test restoring a snapshot from an old version. But as far as I understand the situation, we do not assume all used similarities are actually known here, but just defer the error to a point when we actually know all available similarities.

Copy link
Member

@rjernst rjernst Nov 10, 2017

Choose a reason for hiding this comment

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

Defer is what I meant by "assume". We allow any similarity name to be looked up without error. We don't actually use it, we just need to ensure the lookup does not fail. This is ok because we assume whatever was present when ES last shut down had all known similarities.

@Override
public boolean containsKey(Object key) {
return true;
}

@Override
public SimilarityProvider.Factory get(Object key) {
assert key instanceof String : "key must be a string but was: " + key.getClass();
return SimilarityService.BUILT_IN.get(SimilarityService.DEFAULT_SIMILARITY);
}

@Override
public Set<Entry<String, SimilarityProvider.Factory>> entrySet() {
return Collections.emptySet();
Copy link
Member

@rjernst rjernst Oct 12, 2017

Choose a reason for hiding this comment

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

I realize the analyzer hack does this, but it is inconsistent with what is returned above. I would rather throw a UEO here so that if entrySet begins to be used, we will catch this rather than get weird behavior. (And we should, in a followup or in this PR, fix the other entrySet() impl as well).

Copy link
Contributor Author

@xabbu42 xabbu42 Oct 13, 2017

Choose a reason for hiding this comment

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

I can do that but need to know what an UEO is (I'm not a Java programmer).

Copy link
Member

@jasontedor jasontedor Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@xabbu42 I think that @rjernst meant "UOE" as in UnsupportedOperationException.

Copy link
Member

@rjernst rjernst Oct 13, 2017

Choose a reason for hiding this comment

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

Yes, Jason is correct on what I meant.

Copy link
Contributor Author

@xabbu42 xabbu42 Oct 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I get test failures if make this change. For example:

Suite: org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests
  1> [2017-10-14T16:43:29,230][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgradeFailure]: before test
  1> [2017-10-14T16:43:29,233][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgradeFailure]: after test
  2> REPRODUCE WITH: gradle :core:test -Dtests.seed=67F6816A4DF09ADB -Dtests.class=org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests -Dtests.method="testPluginUpgradeFailure" -Dtests.security.manager=true -Dtests.locale=sr -Dtests.timezone=Asia/Katmandu
FAILURE 0.03s | MetaDataIndexUpgradeServiceTests.testPluginUpgradeFailure <<< FAILURES!
   > Throwable #1: org.junit.ComparisonFailure: expected:<[unable to upgrade the mappings for the index [[foo/BOOM]]]> but was:<[Cannot upgrade index foo]>
   >    at __randomizedtesting.SeedInfo.seed([67F6816A4DF09ADB:37D52F4B7D45621E]:0)
   >    at org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeServiceTests.testPluginUpgradeFailure(MetaDataIndexUpgradeServiceTests.java:141)
   >    at java.lang.Thread.run(Thread.java:748)
  1> [2017-10-14T16:43:29,258][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgrade]: before test
  1> [2017-10-14T16:43:29,263][INFO ][o.e.c.m.MetaDataIndexUpgradeServiceTests] [testPluginUpgrade]: after test

Copy link
Member

@rjernst rjernst Nov 10, 2017

Choose a reason for hiding this comment

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

I think this happens because IndexAnalyzers iterates over the analyzers/normalizers when in close(). Go ahead and add back the empty map, but please add a comment noting the reason?

}
};
SimilarityService similarityService = new SimilarityService(indexSettings, null, similarityMap);
final NamedAnalyzer fakeDefault = new NamedAnalyzer("default", AnalyzerScope.INDEX, new Analyzer() {
@Override
protected TokenStreamComponents createComponents(String fieldName) {
Expand Down