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

Improve similarity integration. #29187

Merged
merged 3 commits into from
Apr 3, 2018
Merged

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Mar 21, 2018

This improves the way similarities are plugged in in order to:

  • reject the classic similarity on 7.x indices and emit a deprecation
    warning otherwise
  • reject unkwown parameters on 7.x indices and emit a deprecation
    warning otherwise

Even though this breaks the plugin API, I'd like to backport to 7.x so
that users can get deprecation warnings when they are doing something
that will become unsupported in the future.

Closes #23208
Closes #29035

@jpountz jpountz added >enhancement :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.3.0 labels Mar 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

This improves the way similarities are plugged in in order to:
 - reject the classic similarity on 7.x indices and emit a deprecation
   warning otherwise
 - reject unkwown parameters on 7.x indices and emit a deprecation
   warning otherwise

Even though this breaks the plugin API, I'd like to backport to 7.x so
that users can get deprecation warnings when they are doing something
that will become unsupported in the future.

Closes elastic#23208
Closes elastic#29035
@jpountz jpountz force-pushed the cleanup/similarities branch from ef78d29 to 6737bf5 Compare March 21, 2018 10:31
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Thanks for removing the classic similarity.
Regarding the validation of setting, I wonder if we should not try to change the format for similarity so that we can validate them directly in the settings framework. Since this change targets 7 only I think it's a good time to make such a change.
For instance we could use an affix setting like index.similarity.*.BM25.b and index.similarity.*.BM25.k1 that would not require to add special logic for validation ?

@jpountz
Copy link
Contributor Author

jpountz commented Mar 21, 2018

@jimczi What about digging this idea in a follow-up change?

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

We discussed with @jpountz and I understand the intent better now, it's important to have this in 6.x if we want to refuse unknown parameter in 7 so I am fine if we do the validation in the similarity service. We can follow up on this to see how we could fold this validation in the settings framework more naturally.
Also the parent_join tests fail because they still use the classic similarity, you'll need to remove it from these tests ;).

@jpountz jpountz merged commit 569d0c0 into elastic:master Apr 3, 2018
@jpountz jpountz deleted the cleanup/similarities branch April 3, 2018 14:45
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2018
* master:
  Reindex: Fix error in delete-by-query rest spec (elastic#29318)
  Improve similarity integration. (elastic#29187)
  Fix some query extraction bugs. (elastic#29283)
  [Docs] Correct experimental note formatting
  Move Nullable into core (elastic#29341)
  [Docs] Update getting-started.asciidoc (elastic#29294)
  Elasticsearch 6.3.0 is now on Lucene 7.3.
  [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677)
  Fix missing comma in ingest-node.asciidoc (elastic#29343)
  Improve exception handling on TransportMasterNodeAction (elastic#29314)
  Don't break allocation if resize source index is missing (elastic#29311)
  Use fixture to test repository-s3 plugin (elastic#29296)
  Fix NDCG for empty search results (elastic#29267)
  Pass through script params in scripted metric agg (elastic#29154)
  Fix Eclipse build.
  Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298)
  Painless: Remove extraneous INLINE constant. (elastic#29340)
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Apr 3, 2018
This improves the way similarities are plugged in in order to:
 - reject the classic similarity on 7.x indices and emit a deprecation
   warning otherwise
 - reject unkwown parameters on 7.x indices and emit a deprecation
   warning otherwise

Even though this breaks the plugin API, I'd like to backport to 7.x so
that users can get deprecation warnings when they are doing something
that will become unsupported in the future.

Closes elastic#23208
Closes elastic#29035
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2018
* master:
  Build: Fix Java9 MR build (elastic#29312)
  Reindex: Fix error in delete-by-query rest spec (elastic#29318)
  Improve similarity integration. (elastic#29187)
  Fix some query extraction bugs. (elastic#29283)
  [Docs] Correct experimental note formatting
  Move Nullable into core (elastic#29341)
  [Docs] Update getting-started.asciidoc (elastic#29294)
  Elasticsearch 6.3.0 is now on Lucene 7.3.
  [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677)
  Fix missing comma in ingest-node.asciidoc (elastic#29343)
  Improve exception handling on TransportMasterNodeAction (elastic#29314)
  Don't break allocation if resize source index is missing (elastic#29311)
  Use fixture to test repository-s3 plugin (elastic#29296)
  Fix NDCG for empty search results (elastic#29267)
  Pass through script params in scripted metric agg (elastic#29154)
  Fix Eclipse build.
  Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298)
  Painless: Remove extraneous INLINE constant. (elastic#29340)
  Remove HTTP max content length leniency (elastic#29337)
  Begin moving XContent to a separate lib/artifact (elastic#29300)
jpountz added a commit that referenced this pull request Apr 4, 2018
This improves the way similarities are plugged in in order to:
 - reject the classic similarity on 7.x indices and emit a deprecation
   warning otherwise
 - reject unkwown parameters on 7.x indices and emit a deprecation
   warning otherwise

Even though this breaks the plugin API, I'd like to backport to 7.x so
that users can get deprecation warnings when they are doing something
that will become unsupported in the future.

Closes #23208
Closes #29035
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 4, 2018
* 6.x:
  Improve similarity integration. (elastic#29187)
  Fix some query extraction bugs. (elastic#29283)
  Fixed quote_field_suffix in query_string (elastic#29332)
  TEST: Update negative byte size setting error msg
  Fix bwc in GeoDistanceQuery serialization (elastic#29325)
martijnvg added a commit that referenced this pull request Apr 5, 2018
* es/master: (68 commits)
  Allow using distance measure in the geo context precision (#29273)
  Disable failing query in QueryBuilderBWCIT.
  Fixed quote_field_suffix in query_string (#29332)
  Use fixture to test repository-url module (#29355)
  Remove undocumented action.master.force_local setting (#29351)
  Enhance error for out of bounds byte size settings (#29338)
  Fix QueryAnalyzerTests.
  Fix HasChildQueryBuilderTests to not use the `classic` similarity.
  [Docs] Correct javadoc of GetIndexRequest (#29364)
  Make TransportRankEvalAction members final
  Add awaits fix for a query analyzer test
  Check presence of multi-types before validating new mapping (#29316)
  Add awaits fix for HasChildQueryBuilderTests
  Remove silent batch mode from install plugin (#29359)
  Align cat thread pool info to thread pool config (#29195)
  Track Lucene operations in engine explicitly (#29357)
  Build: Fix Java9 MR build (#29312)
  Reindex: Fix error in delete-by-query rest spec (#29318)
  Improve similarity integration. (#29187)
  Fix some query extraction bugs. (#29283)
  ...
martijnvg added a commit that referenced this pull request Apr 5, 2018
* es/6.x: (68 commits)
  Add note to migration docs on silent batch mode (#29365)
  Allow using distance measure in the geo context precision (#29273)
  Disable failing query in QueryBuilderBWCIT.
  Improve similarity integration. (#29187)
  Fix some query extraction bugs. (#29283)
  Fixed quote_field_suffix in query_string (#29332)
  TEST: Update negative byte size setting error msg
  Fix bwc in GeoDistanceQuery serialization (#29325)
  Move testMappingConflictRootCause to different class
  Enhance error for out of bounds byte size settings (#29338)
  [Docs] Correct javadoc of GetIndexRequest (#29364)
  Check presence of multi-types before validating new mapping (#29316)
  Make TransportRankEvalAction members final
  Pass through script params in scripted metric agg (#29154)
  Remove silent batch mode from install plugin (#29359)
  Track Lucene operations in engine explicitly (#29357)
  Build: Fix Java9 MR build (#29312)
  Reindex: Fix error in delete-by-query rest spec (#29318)
  Move Nullable into core (#29341)
  [Docs] Correct experimental note formatting
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 5, 2018
* master: (110 commits)
  Remove undocumented action.master.force_local setting (elastic#29351)
  Enhance error for out of bounds byte size settings (elastic#29338)
  Fix QueryAnalyzerTests.
  Fix HasChildQueryBuilderTests to not use the `classic` similarity.
  [Docs] Correct javadoc of GetIndexRequest (elastic#29364)
  Make TransportRankEvalAction members final
  Add awaits fix for a query analyzer test
  Check presence of multi-types before validating new mapping (elastic#29316)
  Add awaits fix for HasChildQueryBuilderTests
  Remove silent batch mode from install plugin (elastic#29359)
  Align cat thread pool info to thread pool config (elastic#29195)
  Track Lucene operations in engine explicitly (elastic#29357)
  Build: Fix Java9 MR build (elastic#29312)
  Reindex: Fix error in delete-by-query rest spec (elastic#29318)
  Improve similarity integration. (elastic#29187)
  Fix some query extraction bugs. (elastic#29283)
  [Docs] Correct experimental note formatting
  Move Nullable into core (elastic#29341)
  [Docs] Update getting-started.asciidoc (elastic#29294)
  Elasticsearch 6.3.0 is now on Lucene 7.3.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants