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

Add configurable max window size #30

Closed

Conversation

iCart
Copy link

@iCart iCart commented Mar 4, 2019

Hello,
This makes the maximum page size configurable.
This is useful when you need to get more than 10000 results from an ECL query for example. (provided you also increase the max window size in the relevant indexes in Elasticsearch).

Another workaround would be to instead use Elasticsearch's scroll API in Snowstorm, but that seems like overkill since that doesn't happen that often.

@codecov-io
Copy link

Codecov Report

Merging #30 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop     #30   +/-   ##
=======================================
  Coverage     74.3%   74.3%           
=======================================
  Files           22      22           
  Lines          144     144           
  Branches         1       1           
=======================================
  Hits           107     107           
  Misses          37      37

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 286d0a0...39f08b3. Read the comment docs.

@kaicode
Copy link
Member

kaicode commented Mar 4, 2019

@iCart Thanks for the idea. We have actually implemented searchAfter functionality in the develop branch. I'm hesitant to accept this because people will not be guaranteed to get all their search results back unless they increase the max window size to many times the recommended value.

Would using search after work for you? Each page has a searchAfter token which you can use to fetch the next page. This has no limit to the number of results you can fetch and has no impact on Elasticsearch memory or performance.

@kaicode kaicode force-pushed the develop branch 2 times, most recently from c1edef2 to 286d0a0 Compare March 4, 2019 21:46
@iCart
Copy link
Author

iCart commented Mar 6, 2019

@kaicode Oh, i didn't realise the searchAfter functionality was already in the develop branch.
After a bit of testing, it does look like it fits our needs.

I'll close this PR then (for the reasons you raised and because it's not needed anymore), thanks!

@iCart iCart closed this Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants