[MOD-12668] Use asynchronous jobs for in GC for SVS#7666
Conversation
alonre24
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7666 +/- ##
==========================================
- Coverage 84.81% 84.78% -0.03%
==========================================
Files 349 349
Lines 54435 54437 +2
Branches 14500 14500
==========================================
- Hits 46168 46155 -13
- Misses 8072 8087 +15
Partials 195 195
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| cur_workers_stats = getWorkersThpoolStats(env) | ||
|
|
||
| # GC background jobs for SVS should be pending in low priority queue. | ||
| env.assertEqual(cur_workers_stats['lowPriorityPendingJobs'], num_workers, message=f"{message_prefix}") |
There was a problem hiding this comment.
Bug: Missing baseline for low priority pending jobs assertion
The test asserts that lowPriorityPendingJobs equals exactly num_workers after invoking GC, but it doesn't capture the baseline lowPriorityPendingJobs value before calling forceInvokeGC. If any low-priority jobs exist in the queue prior to GC invocation (e.g., from background indexing or deletion operations), the assertion will fail because it checks the total count rather than the delta. The code captures totalJobsDone before GC but should also capture lowPriorityPendingJobs to properly verify that GC added exactly num_workers new jobs.
Additional Locations (1)
There was a problem hiding this comment.
The baseline lowPriorityPendingJobs value before calling forceInvokeGC should be zero.
|
backport? |
the backport should use vecsim 8.2, so it would have conflicts either way, I'll create a separate PR. |
* bump vecsim + add test (wip) * Use and test GC jobs * improve test with pauseing threadpool * fix for test_redis_info_modules_vecsim to account for gc in svs * fix cursor comments
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
/backport |
* bump vecsim + add test (wip) * Use and test GC jobs * improve test with pauseing threadpool * fix for test_redis_info_modules_vecsim to account for gc in svs * fix cursor comments (cherry picked from commit 2402970)
|
Successfully created backport PR for |
[MOD-12668] Use asynchronous jobs for in GC for SVS (#7666) * bump vecsim + add test (wip) * Use and test GC jobs * improve test with pauseing threadpool * fix for test_redis_info_modules_vecsim to account for gc in svs * fix cursor comments (cherry picked from commit 2402970) Co-authored-by: alonre24 <[email protected]>
2 participants
Describe the changes in the pull request
Bump vecsim version to the one that reserves threads when we are executing SVS GC, which is using SVS thread pool for parallalism.
This includes extending the debug command that brings stats about the thread pool, so we can test properly that we are submitting jobs after calling the GC.
Mark if applicable
Note
Expose
numJobsInProgressin FT.DEBUG WORKERS stats and update VecSim SVS tests/INFO to use prefixes, verify GC async jobs, and correct deletion counts.num_jobs_in_progresstothpool_stats(indeps/thpool/thpool.h/.c) and include it inredisearch_thpool_get_stats.FT.DEBUG WORKERS statsreply withnumJobsInProgress.tests/pytests/test_debug_commands.pyto assertnumJobsInProgressin workers stats.tests/pytests/test_info_modules.pyVecSim section:PREFIX 'doc:'for HNSW/FLAT indexes; insert SVS-only docs.tests/pytests/test_vecsim_svs.pyGC test:Written by Cursor Bugbot for commit 46707cf. This will update automatically on new commits. Configure here.