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

Remove unused parameters for AnalysisRegistry#processAnalyzerFactory #27232

Merged
merged 1 commit into from
Nov 6, 2017

Conversation

liketic
Copy link

@liketic liketic commented Nov 2, 2017

Remove unused parameters for AnalysisRegistry#processAnalyzerFactory and AnalysisRegistry#processNormalizerFactory. Any comments are appreciated. 👍

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -188,9 +183,9 @@ public IndexAnalyzers build(IndexSettings indexSettings) throws IOException {
}

public Map<String, AnalyzerProvider<?>> buildNormalizerFactories(IndexSettings indexSettings) throws IOException {
final Map<String, Settings> noralizersSettings = indexSettings.getSettings().getGroups("index.analysis.normalizer");
final Map<String, Settings> normalizersSettings = indexSettings.getSettings().getGroups("index.analysis.normalizer");
Copy link
Author

Choose a reason for hiding this comment

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

Fix a typo.

@@ -510,11 +491,9 @@ public IndexAnalyzers build(IndexSettings indexSettings,
unmodifiableMap(analyzers), unmodifiableMap(normalizers));
}

private void processAnalyzerFactory(DeprecationLogger deprecationLogger,
IndexSettings indexSettings,
private void processAnalyzerFactory(IndexSettings indexSettings,
Copy link
Author

Choose a reason for hiding this comment

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

Remove unused deprecationLogger and indexSettings and analyzerAliases .

tokenizerFactoryFactories.get("keyword"), tokenFilterFactoryFactories, charFilterFactoryFactories);
}
for (Map.Entry<String, NamedAnalyzer> entry : analyzerAliases.entrySet()) {
Copy link
Author

Choose a reason for hiding this comment

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

analyzerAliases is never updated.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@liketic thanks for cleaning this up, it looks like most if these were leftovers from removing the 2.0 backward compatibility layer in #26245.
The change looks good to me, I will run tests and merge once they pass.

@cbuescher cbuescher self-assigned this Nov 3, 2017
@cbuescher
Copy link
Member

@liketic CI doesn't seem happy, although the error doesn't seem to be related to your changes, could you either rebase your branch or merge in master before I kick off another build?

…and AnalysisRegistry#processNormalizerFactory
@liketic liketic force-pushed the remove-unused-parameters branch from 275246e to b5e8c4e Compare November 3, 2017 16:19
@liketic
Copy link
Author

liketic commented Nov 3, 2017

@cbuescher Rebase done. Please try again.

@cbuescher
Copy link
Member

@elasticmachine test this please

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Thanks, I will merge this in

@cbuescher cbuescher merged commit 76f81e0 into elastic:master Nov 6, 2017
@liketic liketic deleted the remove-unused-parameters branch November 6, 2017 08:50
jasontedor added a commit that referenced this pull request Nov 7, 2017
* master: (25 commits)
  Disable bwc tests in preparation of backporting #26931
  TemplateUpgradeService should only run on the master (#27294)
  Die with dignity while merging
  Fix profiling naming issues (#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (#27283)
  Add an active Elasticsearch WordPress plugin link (#27279)
  Setting url parts as required to reflect the code base (#27263)
  keys in aggs percentiles need to be in quotes. (#26905)
  Align routing param type with search.json (#26958)
  Update to support bulk updates by query (#27172)
  Remove duplicated SnapshotStatus (#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535)
  Upgrade to Jackson 2.8.10 (#27230)
  Fix inconsistencies in the rest api specs for `tasks` (#27163)
  Adjust RestHighLevelClient method modifiers (#27238)
  Remove unused parameters in AnalysisRegistry (#27232)
  Add more information on `_failed_to_convert_` exception (#27034)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 7, 2017
* ccr: (127 commits)
  Disable bwc tests in preparation of backporting elastic#26931
  TemplateUpgradeService should only run on the master (elastic#27294)
  Die with dignity while merging
  Fix profiling naming issues (elastic#27133)
  Correctly encode warning headers
  Fixed references to Multi Index Syntax (elastic#27283)
  Add an active Elasticsearch WordPress plugin link (elastic#27279)
  Setting url parts as required to reflect the code base (elastic#27263)
  keys in aggs percentiles need to be in quotes. (elastic#26905)
  Align routing param type with search.json (elastic#26958)
  Update to support bulk updates by query (elastic#27172)
  Remove duplicated SnapshotStatus (elastic#27276)
  add split index reference in indices.asciidoc
  Add ability to split shards (elastic#26931)
  [Docs] Fix minor paragraph indentation error for multiple Indices params (elastic#25535)
  Upgrade to Jackson 2.8.10 (elastic#27230)
  Fix inconsistencies in the rest api specs for `tasks` (elastic#27163)
  Adjust RestHighLevelClient method modifiers (elastic#27238)
  Remove unused parameters in AnalysisRegistry (elastic#27232)
  Add more information on `_failed_to_convert_` exception (elastic#27034)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants