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

Deprecate `index.analysis.analyzer.default_index` in favor of `index.analysis.analyzer.default`. #14027

Merged
merged 1 commit into from Oct 12, 2015

Conversation

@jpountz
Copy link
Contributor

commented Oct 8, 2015

Close #11861

public NamedAnalyzer defaultAnalyzer() {
return defaultAnalyzer;
}

public NamedAnalyzer defaultIndexAnalyzer() {

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 8, 2015

Contributor

hmm don't you wanna remove the defaultIndexAnalyzer() method?

This comment has been minimized.

Copy link
@jpountz

jpountz Oct 8, 2015

Author Contributor

I wanted to be consistent with MappedFieldType, which takes an analyzer and a search_analyzer as settings but still exposes an indexAnalyzer() and a searchAnalyzer() in its API

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 8, 2015

Contributor

fair enough

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

LGTM

@rjernst
rjernst reviewed Oct 8, 2015
View changes
core/src/main/java/org/elasticsearch/index/analysis/AnalysisService.java Outdated
if (defaultAnalyzer == null) {
throw new IllegalArgumentException("no default analyzer configured");
}
if (analyzers.containsKey("default_index") && Version.indexCreated(indexSettings).onOrAfter(Version.V_2_0_0)) {

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 8, 2015

Member

Why 2.0+? Shouldn't this be (for master):

if (<3.0) {
  // log deprecation
} else {
  // error
}

Even for backporting, if a user has an old index, it should still show up as deprecated?

This comment has been minimized.

Copy link
@jpountz

jpountz Oct 8, 2015

Author Contributor

I had planned to completely remove it for master, should I keep the error instead?

This comment has been minimized.

Copy link
@rjernst

rjernst Oct 8, 2015

Member

If we are deprecating now, we either need to upgrade the mappings in master, or throw an error? The current code would allow still specifying the setting? I think we should error on new indexes (at minimum 3.0, although if we should error for 2.2+ or something, I dunno). But the deprecation warning should be for all indexes that still "support" this setting.

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2015

@rjernst I pushed more commits to address your comments

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

@rjernst I pushed a new commit.

@rjernst

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

LGTM, one very minor comment on the test.

…analysis.analyzer.default`.

Close #11861
@jpountz jpountz force-pushed the jpountz:deprecate/default_index branch to d3aa356 Oct 12, 2015
jpountz added a commit that referenced this pull request Oct 12, 2015
Deprecate `index.analysis.analyzer.default_index` in favor of `index.analysis.analyzer.default`.
@jpountz jpountz merged commit 08f7f7f into elastic:master Oct 12, 2015
1 check passed
1 check passed
CLA Commit author is a member of Elasticsearch
Details
@jpountz jpountz deleted the jpountz:deprecate/default_index branch Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.