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

Limit the size of the result window to a dynamic property #13188

Merged
merged 1 commit into from Sep 10, 2015

Conversation

Projects
None yet
4 participants
@nik9000
Contributor

nik9000 commented Aug 28, 2015

Requesting a million hits, or page 100,000 is always a bad idea, but users may not be aware of this. This adds a per-index limit on the maximum size + from that can be requested which defaults to 10,000.

This should not interfere with deep-scrolling.

Closes #9311

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Aug 28, 2015

Still a bit of a work in progress but ok for review.

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Aug 28, 2015

Still a bit of a work in progress but ok for review.

And now it seems to be working pretty well.

long resultWindow = from + size;
// We need settingsService's view of the settings because its dynamic.
// indexService's isn't.
int maxResultWindow = indexService.settingsService().getSettings().getAsInt(MAX_RESULT_WINDOW, Defaults.MAX_RESULT_WINDOW);

This comment has been minimized.

@dakrone

dakrone Aug 29, 2015

Member

So I know we don't expect anyone to set this to a crazy high value, but I think we should make it a long just because someone is liable to set it to Integer.MAX_VALUE + 1 and then wonder why it shows negative due to wrapping.

This comment has been minimized.

@jpountz

jpountz Sep 1, 2015

Contributor

+1

This comment has been minimized.

@nik9000

nik9000 Sep 1, 2015

Contributor

I configure the settings validator as POSITIVE_INTEGER below. Is that enough?

@clintongormley

This comment has been minimized.

Member

clintongormley commented Aug 29, 2015

@nik9000 could you add some description of the PR to make it easier to understand? This PR is what the change log links to.

"Result window is too large, from + size must be less than or equal to: [" + maxResultWindow + "] but was ["
+ resultWindow + "]. See the scroll api for a more efficient way to request large data sets. "
+ "This limit can be set by changing the [" + DefaultSearchContext.MAX_RESULT_WINDOW
+ "] index level parameter.");
}
}

This comment has been minimized.

@clintongormley
@clintongormley

This comment has been minimized.

Member

clintongormley commented Aug 29, 2015

Could you document this setting here as well please: https://www.elastic.co/guide/en/elasticsearch/reference/2.0/index-modules.html

@jpountz

View changes

docs/reference/search/request/from-size.asciidoc Outdated
@@ -19,3 +19,7 @@ defaults to `10`.
}
}
--------------------------------------------------
Note that `from` + `size` can not be more than the `index.max_result_window`
index setting which defaults to 10,000. See the scroll api for more efficient

This comment has been minimized.

@jpountz

jpountz Sep 1, 2015

Contributor

maybe put a link from "scroll" to the scroll docs?

@jpountz

This comment has been minimized.

Contributor

jpountz commented Sep 1, 2015

Woohoo! LGTM

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 1, 2015

@nik9000 could you add some description of the PR to make it easier to understand? This PR is what the change log links to.

Done.

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 1, 2015

Could you document this setting here as well please: https://www.elastic.co/guide/en/elasticsearch/reference/2.0/index-modules.html

Done

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 2, 2015

Ok - I think this is pretty much ready. There is still an open question above about long for the setting. I use a POSITIVE_INTEGER validator so I think that is enough, right? I can add a test to the setting that tries to set it to Integer.MAX_VALUE + 1 and notices the error if you'd like.

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 4, 2015

Ping @dakrone and @jpountz - this has one open discussion point but seems otherwise ready. Can you have a look above? Its the long vs int discussion.

@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 9, 2015

Ping @dakrone and @jpountz again - I think this is just waiting on an answer from you to the question above about int vs long?

@jpountz

This comment has been minimized.

Contributor

jpountz commented Sep 9, 2015

Sorry I totally missed this message. The validator should work indeed, +1 to push.

[search] Limit the size of the result window
Requesting a million hits, or page 100,000 is always a bad idea, but users
may not be aware of this. This adds a per-index limit on the maximum size +
from that can be requested which defaults to 10,000.

This should not interfere with deep-scrolling.

Closes #9311
@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 10, 2015

Sorry I totally missed this message. The validator should work indeed, +1 to push.

Cool! I've squashed, rebased, and made some minor corrections to deal with COUNT not being a search type any more.

nik9000 added a commit that referenced this pull request Sep 10, 2015

Merge pull request #13188 from nik9000/limit_on_size
Limit the size of the result window to a dynamic property

@nik9000 nik9000 merged commit 766a25e into elastic:master Sep 10, 2015

1 check passed

CLA Commit author has signed the CLA
Details
@nik9000

This comment has been minimized.

Contributor

nik9000 commented Sep 10, 2015

Pushed to master and 2.1.

@clintongormley

This comment has been minimized.

Member

clintongormley commented Sep 19, 2015

w00t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment