Skip to content

MOD-7329: Determine if fields and documents are expired before returning them as valid results#4858

Merged
kei-nan merged 34 commits intomasterfrom
jk_hfe_search_solution
Sep 15, 2024
Merged

MOD-7329: Determine if fields and documents are expired before returning them as valid results#4858
kei-nan merged 34 commits intomasterfrom
jk_hfe_search_solution

Conversation

@kei-nan
Copy link
Copy Markdown
Collaborator

@kei-nan kei-nan commented Jul 11, 2024

  1. Add an internal ttl table to contain document and field expiration information
    • there will be a thread local timestamp that we check the expiration times against
    • it will be updated when a batch of results is obtained from the pipeline
  2. In our field related iterators check if a field is expired before returning the document as a possible result
    • In query and hybrid iterators it required adding a loop, we might need to consider handling timeout
    • note it will require a lot of fields being expired at roughly the same time
  3. In our result processor check if the document itself was expired
  4. This set of changes helps in the case of lazy expiration to some degree.
    • if the document/field expires between us retrieving it and opening it and retrieving its fields then the problematic edge case remains.
    • We will need a change at the redis level to be able to solve the problem 100%.

A clear and concise description of what the PR is solving, including:

  1. Documents are expired during the query in a lazy fashion which causes empty documents to be returned to the user
    • For fields the issue is even trickier since we can return what the user perceive as incorrect results.
  2. Try and minimize the edge case by checking for the expiration before returning a result up the chain
  3. Less empty documents will be returned to the user.

Which issues this PR fixes

  1. MOD-7329

Main objects this PR modified

  1. Iterators relating to fields
  2. Document Table

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@kei-nan kei-nan force-pushed the jk_hfe_search_solution branch from d53be30 to 7fae25e Compare July 11, 2024 12:54
@kei-nan kei-nan requested review from GuyAv46 and raz-mon July 11, 2024 12:55
@kei-nan kei-nan force-pushed the jk_hfe_search_solution branch 6 times, most recently from ef277e8 to b07f62d Compare July 16, 2024 07:28
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 94.18103% with 27 lines in your changes missing coverage. Please review.

Project coverage is 86.18%. Comparing base (428f802) to head (1a62cdd).
Report is 22 commits behind head on master.

Files with missing lines Patch % Lines
src/debug_commands.c 84.84% 5 Missing ⚠️
src/ttl_table.c 93.05% 5 Missing ⚠️
src/hybrid_reader.c 87.09% 4 Missing ⚠️
src/inverted_index.c 90.00% 4 Missing ⚠️
src/query_optimizer.c 25.00% 3 Missing ⚠️
src/spec.c 92.30% 3 Missing ⚠️
src/numeric_index.c 83.33% 2 Missing ⚠️
src/geometry/query_iterator.cpp 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4858      +/-   ##
==========================================
+ Coverage   86.12%   86.18%   +0.05%     
==========================================
  Files         192      193       +1     
  Lines       34416    34704     +288     
==========================================
+ Hits        29642    29910     +268     
- Misses       4774     4794      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kei-nan kei-nan force-pushed the jk_hfe_search_solution branch 5 times, most recently from 30a1c7a to 5a72cd3 Compare July 16, 2024 13:56
@kei-nan kei-nan marked this pull request as draft July 16, 2024 14:44
@kei-nan kei-nan self-assigned this Jul 16, 2024
@kei-nan kei-nan marked this pull request as ready for review July 17, 2024 09:12
@kei-nan kei-nan removed request for GuyAv46 and raz-mon July 17, 2024 09:12
@kei-nan kei-nan force-pushed the jk_hfe_search_solution branch 5 times, most recently from 86c301f to 9eb1e30 Compare July 17, 2024 15:27
@kei-nan kei-nan requested review from GuyAv46 and raz-mon and removed request for GuyAv46 and raz-mon July 17, 2024 16:00
@kei-nan kei-nan force-pushed the jk_hfe_search_solution branch from 170b87e to 1f73df0 Compare July 18, 2024 16:17
raz-mon
raz-mon previously approved these changes Sep 10, 2024
Copy link
Copy Markdown
Collaborator

@raz-mon raz-mon left a comment

Choose a reason for hiding this comment

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

LGTM - with the following caveat: This PR needs to pass benchmarks (ideally with new scenarios with TTLs) before being released.

@kei-nan
Copy link
Copy Markdown
Collaborator Author

kei-nan commented Sep 29, 2024

/backport

@github-actions
Copy link
Copy Markdown

Backport failed for 2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.10
git worktree add -d .worktree/backport-4858-to-2.10 origin/2.10
cd .worktree/backport-4858-to-2.10
git switch --create backport-4858-to-2.10
git cherry-pick -x 4a67a87bbe11cdb8d4d852ee3b71fd21824f4047

@kei-nan
Copy link
Copy Markdown
Collaborator Author

kei-nan commented Oct 13, 2024

/backport

@redisearch-backport-pull-request
Copy link
Copy Markdown
Contributor

Backport failed for 2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.10
git worktree add -d .worktree/backport-4858-to-2.10 origin/2.10
cd .worktree/backport-4858-to-2.10
git switch --create backport-4858-to-2.10
git cherry-pick -x 4a67a87bbe11cdb8d4d852ee3b71fd21824f4047

@redisearch-backport-pull-request
Copy link
Copy Markdown
Contributor

Backport failed for 8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 8.0
git worktree add -d .worktree/backport-4858-to-8.0 origin/8.0
cd .worktree/backport-4858-to-8.0
git switch --create backport-4858-to-8.0
git cherry-pick -x 4a67a87bbe11cdb8d4d852ee3b71fd21824f4047

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