Added deprecation warning for rescore in scroll queries #33070

not-napoleon · 2018-08-22T21:52:57Z

This adds a deprecation warning for using rescore on scroll queries in 6.x. As per #31775 we will not be supporting this going forward.

See also #32918 which implements the validation error for 7.0

elasticmachine · 2018-08-22T21:52:59Z

Pinging @elastic/es-search-aggs

not-napoleon · 2018-08-22T21:53:52Z

I'm not sure what the correct version tag to put on this PR is.

jtibshirani

It looks good to me overall, a couple comments:

It would be great to add a test for the deprecation logging using ESTestCase#assertWarnings.
As we did for 7.0.0, we can add a small note to migration_6_5.asciidoc explaining the deprecation.

It might be helpful to look at #30157 as an example of adding a test + migration notes. As for the PR label, I think we can just add 6.5.0.

jtibshirani · 2018-08-23T18:29:11Z

server/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -137,6 +137,10 @@ public ActionRequestValidationException validate() {
        if (source != null && source.size() == 0 && scroll != null) {
            validationException = addValidationError("[size] cannot be [0] in a scroll context", validationException);
        }
+        if (source != null && source.rescores() != null && source.rescores().isEmpty() == false && scroll != null) {
+            DEPRECATION_LOGGER.deprecated("Using [rescore] for a scroll query is deprecated and will return a 400 error in future " +


It would be nice to mention that the rescore will be ignored, as that may not be clear to the user only from the fact it's deprecated.

jtibshirani

A couple last comments.

jtibshirani · 2018-08-23T22:32:24Z

server/src/test/java/org/elasticsearch/search/SearchRequestTests.java

+        {
+            // Rescore is deprecated on scroll requests
+            SearchRequest searchRequest = createSearchRequest().source(new SearchSourceBuilder());
+            searchRequest.source().addRescorer(new NoOpRescorerBuilder());


Instead of creating a new builder class, I think we could do something like new QueryRescorerBuilder(QueryBuilders.matchAllQuery()));.

maybe this could also be done on the other PR which will go to master and save us some lines of code there too ;)

@jtibshirani Thanks for pointing this out to me, that's exactly what I needed, and failed to find!

jtibshirani · 2018-08-23T22:46:38Z

docs/reference/migration/migrate_6_0/search.asciidoc

@@ -180,6 +180,11 @@ In prior versions the source field names were relative to the inner hit.
 The `from` parameter can no longer be used in the search request body when initiating a scroll.
 The parameter was already ignored in these situations, now in addition an error is thrown.

+Using `rescore` with a scroll query now raises a deprecation warning and


I think this note should instead go in migrate_6_5.asciidoc.

javanna

left a couple of minor comments, LGTM otherwise

javanna · 2018-08-24T07:30:10Z

docs/reference/migration/migrate_6_0/search.asciidoc

@@ -180,6 +180,11 @@ In prior versions the source field names were relative to the inner hit.
 The `from` parameter can no longer be used in the search request body when initiating a scroll.
 The parameter was already ignored in these situations, now in addition an error is thrown.

+Using `rescore` with a scroll query now raises a deprecation warning and
+ignores the parameter.  In earlier 6.x releases, rescore on scroll queries was
+silently ignored.  In future versions, including we will return a `400 - Bad


be more specific on when? next major version? Say "From 7.0 on, a 400 - Bad Request will be returned instead.

javanna · 2018-08-24T07:31:07Z

server/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -137,6 +137,10 @@ public ActionRequestValidationException validate() {
        if (source != null && source.size() == 0 && scroll != null) {
            validationException = addValidationError("[size] cannot be [0] in a scroll context", validationException);
        }
+        if (source != null && source.rescores() != null && source.rescores().isEmpty() == false && scroll != null) {
+            DEPRECATION_LOGGER.deprecated("Using [rescore] for a scroll query is deprecated and will be ignored. Future versions will " +


shall we be more specific on when this will become a hard error?

javanna · 2018-08-24T07:32:23Z

server/src/test/java/org/elasticsearch/search/SearchRequestTests.java

+        {
+            // Rescore is deprecated on scroll requests
+            SearchRequest searchRequest = createSearchRequest().source(new SearchSourceBuilder());
+            searchRequest.source().addRescorer(new NoOpRescorerBuilder());


maybe this could also be done on the other PR which will go to master and save us some lines of code there too ;)

not-napoleon · 2018-08-27T18:39:52Z

retest this please

Conflicts: docs/reference/migration/migrate_6_5.asciidoc

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

Added deprecation warning for rescore in scroll queries

e9c9c10

not-napoleon added >bug >deprecation :Search Relevance/Ranking Scoring, rescoring, rank evaluation. labels Aug 22, 2018

not-napoleon requested a review from javanna August 23, 2018 17:08

jtibshirani reviewed Aug 23, 2018

View reviewed changes

Respons to PR feedback

1721d48

jtibshirani reviewed Aug 23, 2018

View reviewed changes

javanna approved these changes Aug 24, 2018

View reviewed changes

not-napoleon added 2 commits August 24, 2018 14:38

Response to PR feedback

23b4f52

fixed import ordering

6f3a7af

not-napoleon added 3 commits August 27, 2018 14:41

Merge branch '6.x' into fix/deprecate-rescore-in-scroll-queries

7f75b7b

Conflicts: docs/reference/migration/migrate_6_5.asciidoc

linked search changes section in docs

cfa6870

Merge branch '6.x' into fix/deprecate-rescore-in-scroll-queries

663255c

not-napoleon merged commit 7d4895d into elastic:6.x Aug 29, 2018

not-napoleon deleted the fix/deprecate-rescore-in-scroll-queries branch August 29, 2018 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added deprecation warning for rescore in scroll queries #33070

Added deprecation warning for rescore in scroll queries #33070

not-napoleon commented Aug 22, 2018

elasticmachine commented Aug 22, 2018

not-napoleon commented Aug 22, 2018

jtibshirani left a comment

jtibshirani Aug 23, 2018

jtibshirani left a comment

jtibshirani Aug 23, 2018

javanna Aug 24, 2018

not-napoleon Aug 24, 2018

jtibshirani Aug 23, 2018

javanna left a comment

javanna Aug 24, 2018

javanna Aug 24, 2018

javanna Aug 24, 2018

not-napoleon commented Aug 27, 2018

Added deprecation warning for rescore in scroll queries #33070

Added deprecation warning for rescore in scroll queries #33070

Conversation

not-napoleon commented Aug 22, 2018

elasticmachine commented Aug 22, 2018

not-napoleon commented Aug 22, 2018

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-napoleon commented Aug 27, 2018