-
Notifications
You must be signed in to change notification settings - Fork 17
Allow paginated search using ES's search_after in SearchResult:fetchNext #233
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
|
@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 |
There was a problem hiding this 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
|
Simple enough: this means This requires some documentation though, but other than that, it's fine by me. |
|
PR updated, scroll is now prioritized as long as the user specifies a scroll param. |
Introduces
Related issue
kuzzleio/kuzzle#856
Boyscoot