Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4869 +/- ##
==========================================
- Coverage 86.14% 86.08% -0.07%
==========================================
Files 192 192
Lines 34256 34418 +162
==========================================
+ Hits 29511 29629 +118
- Misses 4745 4789 +44 ☔ View full report in Codecov by Sentry. |
8e64fe0 to
70fb1c2
Compare
| REPLY_KVSTR_SAFE("payload_field", rule->payload_field); | ||
| } | ||
|
|
||
| if (rule->index_all) { |
There was a problem hiding this comment.
Maybe we should also output here the memory usage specifically for the new inverted index in case it was enabled?
There was a problem hiding this comment.
I don't think we should add that here.
This may be added to the FT.INFO response, but I don't want it to create confusion with regarding the memory we use, since this memory is counted already in sp->stats.invertedSize
| if (rule->index_all) { | ||
| REPLY_KVSTR_SAFE("indexes_all", "true"); | ||
| } else { | ||
| REPLY_KVSTR_SAFE("indexes_all", "false"); |
There was a problem hiding this comment.
maybe use the same strings you used for the input arguments.
i.e enable and disable
just to be consistent
There was a problem hiding this comment.
Thing is that this needs to be lower case and separated by _ (to be consistent with the rest of the response), so this can become index_all to more resemble the creation argument format. Then it doesn't really matter anymore. But I can go with index_all..
4e879a2 to
be5e1cf
Compare
MeirShpilraien
left a comment
There was a problem hiding this comment.
Looks good, few comments and some other general comments:
- How do the tests verifies that the new feature is been used, they would have pass even with the old code right?
- Lets improve the top comment with some additional info:
- Explanation of the problem we have today, at what scenarios it happened.
- How do we fix it.
- The new API that was added.
Wildcard and NOT iterators latency fix
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.8
git worktree add -d .worktree/backport-4869-to-2.8 origin/2.8
cd .worktree/backport-4869-to-2.8
git switch --create backport-4869-to-2.8
git cherry-pick -x 428f8023e507dc9a9015a9d129093d473dd3d9f1 |
|
Successfully created backport PR for |
Wildcard and NOT iterators latency fix (cherry picked from commit 428f802)
Describe the changes in the pull request
This PR aspires to fix the latency issue we have with our wildcard and NOT iterators, in which the doc-id increment is done one-by-one, resulting in a lot of time wasted on looking for un-existing documents.
The problem is encountered when there are many writes and deletions, such that there are big "holes" in the docIds of the existing docs in the database. This caused us to waste time incrementing the docIds and checking whether their corresponding documents exist.
We opt to fix this by adding an "existing-docs" inverted-index, holding the doc-ids of all the currently existing docs (up to GC update), which the wildcard and NOT iterators can utilize to efficiently jump between large doc-id deltas instead of the single-increment jumps used currently.
We use the existing inverted-index API so no new API is introduced.
This inverted index is cleaned by the GC, just like most of the other inverted indexes we hold.
Mark if applicable