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.

Sign up for GitHub

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

Jump to bottom

Fix expire during query #1437

Merged
merged 44 commits into from
Aug 27, 2020
Merged

Fix expire during query #1437

merged 44 commits into from
Aug 27, 2020

Conversation

ashtul
Copy link
Contributor

@ashtul ashtul commented Aug 19, 2020

No description provided.

@codecov
Copy link

codecov bot commented Aug 19, 2020 •
edited
Loading

Codecov Report

Merging #1437 into master will increase coverage by 0.02%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1437      +/-   ##
==========================================
+ Coverage   81.65%   81.68%   +0.02%     
==========================================
  Files         143      143              
  Lines       20669    20674       +5     
==========================================
+ Hits        16878    16887       +9     
+ Misses       3791     3787       -4
Impacted Files Coverage Δ
src/result_processor.c 90.03% <95.23%> (+0.18%) ⬆️
src/rlookup.c 80.00% <0.00%> (+1.48%) ⬆️

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 523c104...2e2f81d. Read the comment docs.

@ashtul
Copy link
Contributor Author

ashtul commented Aug 19, 2020

Tests of ft.aggregate had to be fixed since they returned empty docs.
fix #1438

@rafie
Copy link
Contributor

rafie commented Aug 19, 2020

@ashtul Did you test it on RSCoordinator?

@ashtul
Copy link
Contributor Author

ashtul commented Aug 19, 2020

Current behavior:

  1. An empty hash is received by the redis server.
    * This can happen if the hash was passively-expired during the query.
    * In RediSearch 1.x, if a hash was deleted w/o updating the index.
  2. It is counted in total results
  3. An empty array is returned

Cause:
In serializeResult, we return array the size of which is postponed. If there are no fields, an empty array is returned.

Fix:
In check RLookup has fields before entering the for loop.

@ashtul
Copy link
Contributor Author

ashtul commented Aug 19, 2020

@rafie, It was not tested with the coordinator

@ashtul
Copy link
Contributor Author

ashtul commented Aug 24, 2020 •
edited
Loading

Options:

  • Move total result to the end of list, after e filter expired docs. - breaks api
  • Leave count as is and return expired instead of doc. - not as elegant
  • Aggregate replies on our side instead of using postponed array size - expensive

MeirShpilraien
MeirShpilraien reviewed Aug 25, 2020
View reviewed changes
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 just need documentation on the expire issue.

MeirShpilraien
MeirShpilraien reviewed Aug 26, 2020
View reviewed changes
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Small question, other then this looks good 👍

MeirShpilraien
MeirShpilraien approved these changes Aug 27, 2020
View reviewed changes
Copy link
Collaborator

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍

@ashtul ashtul merged commit 5b6c4ce into master Aug 27, 2020
@ashtul ashtul deleted the fix-expire-during-query branch August 27, 2020 10:56
@rafie rafie added this to Done in 2.0 Aug 27, 2020
MeirShpilraien pushed a commit that referenced this pull request Aug 30, 2020
@ashtul @MeirShpilraien
Fix expire during query (#1437)
30d597a
ashtul added a commit that referenced this pull request Sep 2, 2020
@MeirShpilraien @rafie
@ashtul @gkorland
Deprecate ft.add, ft.del, ft.drop (#1457)
e829c8a 
* Deprecate ft.add, ft.del, ft.drop

* Fix tests

* review fixes

* some more docs fixes

* more docs fixes

* some more fixes

* CircleCI build logs and readies update (#1463)

* Explanation for spellcheck score (#1429)

* [DOC] explain ft.spellcheck scoring

* add assert

* fix assert location

* if instead of assert

* Fix expire during query (#1437)

* Distance function for aggregation APPLY (#1246)

* geo files

* Apply distance for geo

* fix per meir review

* docs

Co-authored-by: Guy Korland <gkorland@gmail.com>

* Search2 docs (#1383)

* Update index.md
* Update Quick_Start.md
* Update Commands.md

Co-authored-by: Ariel Shtul <ashtul@gmail.com>

* fix flaky test

* expanding on HSET

* clear

* extra docs

* temporary

* geo

Co-authored-by: Rafi Einstein <rafi@redislabs.com>
Co-authored-by: Ariel Shtul <ashtul@gmail.com>
Co-authored-by: Guy Korland <gkorland@gmail.com>
@emmanuelkeller emmanuelkeller added this to the 2.0.0 milestone Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafie rafie rafie approved these changes

@MeirShpilraien MeirShpilraien MeirShpilraien approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
2.0
  
Done
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashtul @rafie @MeirShpilraien @emmanuelkeller