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

Scroll queries asking for rescore are considered invalid #32918

Merged
merged 12 commits into from
Aug 28, 2018

Conversation

not-napoleon
Copy link
Member

This PR changes our behavior from silently ignoring rescore in a scroll query to instead report to the user that such a query is invalid.

Closes #31775

@not-napoleon not-napoleon added >bug :Search Relevance/Ranking Scoring, rescoring, rank evaluation. labels Aug 16, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a couple of comments, LGTM otherwise

@@ -188,6 +188,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() && scroll != null) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we tend to prefer source.rescores().isEmpty() == false in our codebase

{
// Rescore is not allowed on scroll requests
SearchRequest searchRequest = createSearchRequest().source(new SearchSourceBuilder());
searchRequest.source().addRescorer(mock(RescorerBuilder.class));
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to mock the rescorer builder, seems like an easy object to create manually, unless I am missing something. In general, I tend to use mockito only as a last-resort, when things are really hard to reconstruct, that's why I'm asking.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left two minors, still LGTM

==== Scroll queries cannot use `rescore` anymore
Including a rescore clause on a query that creates a scroll (`scroll=1m`)
has been deprecated in 6 and will now return a `400 - Bad request`. Allowing
rescore on scroll queries would break the scroll sort. In 6, the rescore
Copy link
Member

Choose a reason for hiding this comment

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

maybe replace 6 with previous versions (only this second occurrence)? also when you mention the version can you specify also the minor version that is was deprecated in ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this language a bit. I want to be careful here, because according to the original ticket, this was allowed at some time in the 5.x line. It's not clear to me when exactly we stopped supporting rescoring scroll queries.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me!

@@ -164,4 +181,33 @@ private static SearchRequest copyRequest(SearchRequest searchRequest) throws IOE
}
return result;
}

private static class NoOpRescorerBuilder extends RescorerBuilder<NoOpRescorerBuilder> {
Copy link
Member

Choose a reason for hiding this comment

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

I had not looked closely now I see why you were using mockito before :) I don't mind if you prefer to go back to it, I am happy either way

@s1monw
Copy link
Contributor

s1monw commented Aug 28, 2018

some checks seem to be stuck. I am +1 merging this... packaging-sample seems hung.

@not-napoleon not-napoleon merged commit 84b61d0 into elastic:master Aug 28, 2018
@not-napoleon not-napoleon deleted the fix/31775 branch August 28, 2018 19:48
dnhatn added a commit that referenced this pull request Aug 29, 2018
* master:
  Painless: Add Bindings (#33042)
  Update version after client credentials backport
  Fix forbidden apis on FIPS (#33202)
  Remote 6.x transport BWC Layer for `_shrink` (#33236)
  Test fix - Graph HLRC tests needed another field adding to randomisation exception list
  HLRC: Add ML Get Records API (#33085)
  [ML] Fix character set finder bug with unencodable charsets (#33234)
  TESTS: Fix overly long lines (#33240)
  Test fix - Graph HLRC test was missing field name to be excluded from randomisation logic
  Remove unsupported group_shard_failures parameter (#33208)
  Update BucketUtils#suggestShardSideQueueSize signature (#33210)
  Parse PEM Key files leniantly (#33173)
  INGEST: Add Pipeline Processor (#32473)
  Core: Add java time xcontent serializers (#33120)
  Consider multi release jars when running third party audit (#33206)
  Update MSI documentation (#31950)
  HLRC: create base timed request class (#33216)
  [DOCS] Fixes command page titles
  HLRC: Move ML protocol classes into client ml package (#33203)
  Scroll queries asking for rescore are considered invalid (#32918)
  Painless: Fix Semicolon Regression (#33212)
  ingest: minor - update test to include dissect (#33211)
  Switch remaining LLREST usage to new style Requests (#33171)
  HLREST: add reindex API (#32679)
not-napoleon added a commit that referenced this pull request Aug 29, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants