Limit the size of the result window to a dynamic property #13188
Conversation
nik9000
added
>breaking
:Search/Search
|
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I configure the settings validator as POSITIVE_INTEGER below. Is that enough?
|
@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."); | ||
| } | ||
| } |
|
Could you document this setting here as well please: https://www.elastic.co/guide/en/elasticsearch/reference/2.0/index-modules.html |
| @@ -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 | |||
There was a problem hiding this comment.
maybe put a link from "scroll" to the scroll docs?
|
Woohoo! LGTM |
Done. |
Done |
|
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. |
|
Sorry I totally missed this message. The validator should work indeed, +1 to push. |
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 elastic#9311
nik9000
force-pushed
the
limit_on_size
branch
from
76c5d4c
to
e498196
Compare
Cool! I've squashed, rebased, and made some minor corrections to deal with COUNT not being a search type any more. |
Limit the size of the result window to a dynamic property
|
Pushed to master and 2.1. |
|
w00t |
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 + fromthat can be requested which defaults to 10,000.This should not interfere with deep-scrolling.
Closes #9311