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

Remove previous and next cache in SearchResult #177

Merged
merged 6 commits into from
Mar 1, 2017

Conversation

dbengsch
Copy link
Contributor

@dbengsch dbengsch commented Feb 8, 2017

Fix #176

  • Remove _previous and _next cache in SearchResult
  • Remove previous function that could't be reworked properly for a scroll

@codecov-io
Copy link

codecov-io commented Feb 8, 2017

Codecov Report

❗ No coverage uploaded for pull request base (develop@138d393). Click here to learn what that means.

@@            Coverage Diff             @@
##             develop     #177   +/-   ##
==========================================
  Coverage           ?   99.33%           
==========================================
  Files              ?       16           
  Lines              ?     1642           
  Branches           ?      439           
==========================================
  Hits               ?     1631           
  Misses             ?       11           
  Partials           ?        0
Impacted Files Coverage Δ
src/Collection.js 97.37% <100%> (ø)
src/SearchResult.js 97.05% <94.73%> (ø)

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 138d393...c3f7e87. Read the comment docs.

* @param {Collection} dataCollection
* @param {int} total
* @param {Document[]} documents
* @param {object} [aggregations]
* @param {object} [searchArgs]
* @param {KuzzleSearchResult} [previous]
* @property {Collection} dataCollection

Choose a reason for hiding this comment

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

???

Copy link
Contributor Author

@dbengsch dbengsch Feb 8, 2017

Choose a reason for hiding this comment

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

This states the object built by the constructor function has a defined property called dataCollection of type Collection

Choose a reason for hiding this comment

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

OK, my bad. I read "@param" instead of "@Property"

Copy link
Contributor

@xbill82 xbill82 left a comment

Choose a reason for hiding this comment

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

PR needs a more detailed description. I found no mention about next and previous search results in the docs.
Also don't understand why we had these before and why we take them out now.

@dbengsch
Copy link
Contributor Author

dbengsch commented Feb 10, 2017

@xbill82:
Issue about the documentation created kuzzleio/documentation#88 .

@scottinet scottinet changed the base branch from develop to 3.x-patches February 10, 2017 14:42
@scottinet scottinet changed the base branch from 3.x-patches to 3.x February 13, 2017 10:22
@dbengsch
Copy link
Contributor Author

@xbill82 Please refer to the connected ticket to know why we had to remove them.

@xbill82
Copy link
Contributor

xbill82 commented Feb 20, 2017

@dbengsch Thanks, I actually didn't really read the title of the issue (just got annoyed by the "no description" message, but the title is actually enough :) I understand that the problem is that they introduce a memory leak, but I've missed all the discussion that lead to the decision to take them out instead of fixing the mem leak. We can discuss it IRL if u prefer.

@stafyniaksacha stafyniaksacha changed the base branch from 3.x to 4.x February 21, 2017 09:59
@benoitvidis
Copy link
Contributor

@xbill82 This issue is severe enough imo to take immediate actions and remove this pseudo-functionality just right now.

Users can very easily work around the removed previous options on client side by storing the whole dataset in a local collection but this is not what Kuzzle should do on its side by default.

@scottinet
Copy link
Contributor

@xbill82 > this is not a memleak, since the used memory is not lost, but memory consumption. And that's the problem: there is no way to implement this feature without the client consuming too much memory.

It has been decided to make this cursor "forward-only", as are many cursor implementations out there.

@xbill82
Copy link
Contributor

xbill82 commented Mar 1, 2017

@scottinet thanks for the details, now I get the point (and the severity) of the issue.

@xbill82 xbill82 merged commit d5f82a5 into 4.x Mar 1, 2017
@xbill82 xbill82 deleted the topic-176-fix-search-result branch March 1, 2017 08:58
This was referenced Apr 10, 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.

7 participants