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

Allow paginated search using ES's search_after in SearchResult:fetchNext #233

Merged
merged 3 commits into from
Jul 6, 2017

Conversation

samniisan
Copy link
Contributor

@samniisan samniisan commented Jun 16, 2017

Introduces

  • Way to use ES's search_after if conditions are met when calling SearchResult:fetchNext (better than using scrolls, bypasses the default search limit count)

Related issue

kuzzleio/kuzzle#856

Boyscoot

  • Updates README.md (typos / various)

@codecov-io
Copy link

codecov-io commented Jun 16, 2017

Codecov Report

Merging #233 into 5.x will increase coverage by 0.01%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##              5.x     #233      +/-   ##
==========================================
+ Coverage   98.56%   98.57%   +0.01%     
==========================================
  Files          17       17              
  Lines        2085     2457     +372     
  Branches      595      727     +132     
==========================================
+ Hits         2055     2422     +367     
- Misses         30       35       +5
Impacted Files Coverage Δ
src/Collection.js 99.4% <ø> (+0.76%) ⬆️
src/SearchResult.js 95% <72.72%> (-5%) ⬇️
src/Room.js 100% <0%> (ø) ⬆️
src/security/Security.js 97.78% <0%> (+0.03%) ⬆️

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 00dcaca...9acbc17. Read the comment docs.

Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

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

I have an issue with the way this is currently implemented: search_after is silently applied and takes precedence over scroll options.
These two ways of paginating searches may have slightly different behaviors: while scroll is slower and requires a lot more resources on the server than search_after, it also guarantees data consistency. On the other hand, search_after is a quick and consistent enough way to get data, at the price of having a result set reflecting the indexes at different points of time.

The way I see it, these two methods are complementary, and should both be proposed to SDK users, with search_after being the default one, as it should be used most of the time.

And because these two methods might return different result sets, I'm not sure that silently overriding a scroll request with a search_after one is a good thing.

@benoitvidis
Copy link
Contributor

@scottinet I agree on the scroll precedence that should not be overridden but to me, the user already has a way to specify he wants to use scroll over search_after, just by setting the scroll parameter in his search query.

Copy link
Contributor

@benoitvidis benoitvidis left a comment

Choose a reason for hiding this comment

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

same remarks as for php sdk

@scottinet
Copy link
Contributor

scottinet commented Jun 19, 2017

Simple enough: this means search_after is only applied if scrollId is not set.

This requires some documentation though, but other than that, it's fine by me.

@samniisan
Copy link
Contributor Author

PR updated, scroll is now prioritized as long as the user specifies a scroll param.

@benoitvidis benoitvidis merged commit 1d5f2df into 5.x Jul 6, 2017
@benoitvidis benoitvidis deleted the 856-search-after branch July 6, 2017 08:13
@stafyniaksacha stafyniaksacha mentioned this pull request Jul 19, 2017
@scottinet scottinet mentioned this pull request Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants