[MOD-8115] Free spec resources in the main thread by meiravgri · Pull Request #5324 · RediSearch/RediSearch

meiravgri · 2024-12-09T08:26:17Z

This PR fixes flaky class FGCTest cpp tests.
In these tests we create a spec and add documents, potentially causing the global TotalIIBlocks number (global counter of the inverted indexes blocks across the model) to increase.
This number is decreased upon spec deletion in IndexSpec_FreeUnlinkedData
IndexSpec_FreeUnlinkedData is sent to a background thread if RSGlobalConfig.freeResourcesThread is set to its default value - true. In this case tests might start running while the clean-up process of previous tests is still active in the background and we can't rely on the global counters to not include changes made in previous tests, such as TotalIIBlocks number.
To fics that, this PR sets RSGlobalConfig.freeResourcesThread to false in all cpp tests, i.e disables the free resources thread in cpp-tests to ensure all the spec resources, including global objects such as the total number of blocks (TotalIIBlocks), are cleaned once the test has finished.

Additional improvements

Better clean-up of `class FGCTest` test suite:

use pthread_join instead of pthread_detach to wait for the gc cbWrapper
add run flag to hint the gc wrapper thread to exit
move args_t args to class FGCTest test suite members, instead of dynamic allocation.

Allow multiple gc runs in each test

(taken from #5049)

codecov · 2024-12-09T08:48:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.72%. Comparing base (805d3d8) to head (f6d348e).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5324      +/-   ##
==========================================
- Coverage   86.73%   86.72%   -0.02%     
==========================================
  Files         193      193              
  Lines       34704    34704              
==========================================
- Hits        30101    30097       -4     
- Misses       4603     4607       +4

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

allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049

GuyAv46 · 2024-12-09T13:06:39Z

tests/cpptests/test_cpp_forkgc.cpp

@@ -82,17 +88,16 @@ class FGCTest : public ::testing::Test {
  void runGcThread() {
    fgc = reinterpret_cast<ForkGC *>(get_spec(ism)->gc->gcCtx);
    thread = {0};


Is this still needed?

GuyAv46 · 2024-12-09T13:10:07Z

tests/cpptests/test_cpp_forkgc.cpp

@@ -82,17 +88,16 @@ class FGCTest : public ::testing::Test {
  void runGcThread() {
    fgc = reinterpret_cast<ForkGC *>(get_spec(ism)->gc->gcCtx);


Can we move this line to the setup function? IMO it doesn't make much sense to have it here

redisearch-backport-pull-request · 2024-12-09T15:27:52Z

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

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

git fetch origin 2.8
git worktree add -d .worktree/backport-5324-to-2.8 origin/2.8
cd .worktree/backport-5324-to-2.8
git switch --create backport-5324-to-2.8
git cherry-pick -x fc508014c80a3dd7b88340c59d79b53a34008bf2

redisearch-backport-pull-request · 2024-12-09T15:27:53Z

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

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

git fetch origin 2.6
git worktree add -d .worktree/backport-5324-to-2.6 origin/2.6
cd .worktree/backport-5324-to-2.6
git switch --create backport-5324-to-2.6
git cherry-pick -x fc508014c80a3dd7b88340c59d79b53a34008bf2

redisearch-backport-pull-request · 2024-12-09T15:27:55Z

Successfully created backport PR for 2.10:

[2.10] [MOD-8115] Free spec resources in the main thread #5327

* free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801)

redisearch-backport-pull-request · 2024-12-09T15:27:58Z

Successfully created backport PR for 8.0:

[8.0] [MOD-8115] Free spec resources in the main thread #5328

* free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801)

[MOD-8115] Free spec resources in the main thread (#5324) * free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801) Co-authored-by: meiravgri <[email protected]>

* free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801)

[MOD-8115] Free spec resources in the main thread (#5324) * free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801) Co-authored-by: meiravgri <[email protected]>

[MOD-8115] Free spec resources in the main thread (#5324) * free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801)

* [MOD-8115] Free spec resources in the main thread (#5324) * free spec resources in the main thread. * improve cpp gc tests suite: allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049 (cherry picked from commit fc50801) * fix

#5330)" This reverts commit 5f3ce64.

This reverts commit ef25dca. Revert "[2.8] Fix Max Frequency Misscalculation - [MOD-8158] (#5560)" This reverts commit a6864e3. Revert "[2.8] MOD-8561: Fix Inverted Index SeekTo Edge Case (#5542)" This reverts commit bb873af. Revert "[2.8] Fix legacy geofilter leak - [MOD-8568] (#5518)" This reverts commit 979f3d2. Revert "Backport 4243 to 2.8 (#5508)" This reverts commit 9af7959. Revert "MOD-7570 Backport 5422 to 2.8 (#5470)" This reverts commit 540b5f3. Revert "[2.8] MOD-7872: Add Types To Hide User Input (#5489)" This reverts commit b2bf242. Revert "[2.8] MOD-7864: Obfuscation API (#5488)" This reverts commit 4866bf1. Revert "[2.8] MOD-7634: Fix NOSTEM (#5475)" This reverts commit a015a55. Revert "[2.8] [MOD-8462] rename total_active_writes -> total_active_write_threads (#5444)" This reverts commit baadb4d. Revert "[2.8] MOD-8058: Avoid error with _NUM_SSTRING (#5187) (#5427)" This reverts commit 5b49276. Revert "Backport metrics info 2.8 [MOD-8388] (#5407)" This reverts commit f428033. Revert " [2.8] Fixes for inverted indexes encoding - [MOD-8248] (#5389)" This reverts commit 0c7227e. Revert "[2.8] MOD-8221: backport info fields (for metrics) to 2.8 (#5358)" This reverts commit b7eaa56. Revert "[2.8] Improve "Raw doc id" encoding - [MOD-8255] (#5371)" This reverts commit 152043b. Revert "[2.8] Improve SkipToBlock logic - [MOD-8255] (#5369)" This reverts commit c1a2c8f. Revert "[2.8][MOD-8115] Free spec resources in the main thread (#5324) (#5330)" This reverts commit 5f3ce64. Revert "[2.8] Fix Sorting Vector - [MOD-6783] (#5316)" This reverts commit 04f7778. Revert "[2.8] Fix a flakiness in testCursorOnCoordinator - [MOD-8016] (#5305)" This reverts commit 4fc1b27. Revert "[MOD-7949] Avoid lazy expire upon scan keys in background [2.8] (#5303)" This reverts commit 16e4ff4. Revert "[2.8] Fix long PREFIX/SUFFIX/INFIX/WILDCARD queries - [MOD-7882] (#5299)" This reverts commit 09f5b62. Revert "[2.8] Remove assertion in optimizer (#5296)" This reverts commit 45db16a. Revert "[2.8] MOD-8188: Fix OnReopen Comments (#5288)" This reverts commit 001746f. Revert "[2.8] Fix missing expansions in text queries - [MOD-8142] (#5287)" This reverts commit ed13507. Revert "[2.8] Fix Tag OnReopen Callback - [MOD-8011] (#5280)" This reverts commit 40b8012. Revert "[2.8] Fix MRIterator ownership mechanism - [MOD-8108] (#5269)" This reverts commit 90a5685. Revert "[2.8] [MOD-8125] Create inverted index in write operations only (#5260)" This reverts commit ebb9484. Revert "[2.8] MOD-8129: Fix indexed_percent info stat (#5249)" This reverts commit fb9781a. Revert "[2.8] MOD-8097, MOD-8114 Fix memory counting (#5242)" This reverts commit a21a909. Revert "[2.8] Cleanup for the trimming tree logic (#5225)" This reverts commit 1b9126b. Revert "[2.8] [MOD-8035] Add field index error for JSON field indexing failures (#5205)" This reverts commit 3a13ddb. Revert "[2.8] Fix Numeric Tree Balance - [MOD-8081, MOD-8082] (#5200)" This reverts commit a2948c4. Revert "[2.8] Performance improvement in indexBulkFields - [MOD-8093] (#5192)" This reverts commit 2b0e0a0. Revert "[2.8] MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5167)" This reverts commit f860505. Revert "[2.8] Skip test_multithread:test_async_updates_sanity with sanitizer (#5124)" This reverts commit 938369a. Revert "[2.8] Improve Error Message on VecSim Syntax Error - [MOD-7887] (#5108)" This reverts commit ba614ca. Revert "[2.8] Fix flaky test (#5084)" This reverts commit f579156. Revert "[2.8] Fix flaky test_async_updates_sanity - avoid timeout in force invoke GC [MOD-7867] (#5080)" This reverts commit 1a0eabc. Revert "[2.8] Handle VecSim in-place deletion after async [MOD-7643, MOD-7732] (#5048)" This reverts commit fcc19e9.

This reverts commit 2ea41bf. Revert "[2.6] Fix Max Frequency Misscalculation - [MOD-8158] (#5559)" This reverts commit 7e0288d. Revert "[2.6] Fix flakiness in a test - [MOD-8611] (#5546)" This reverts commit 1a43260. Revert "[2.6] MOD-8561: Fix Inverted Index SeekTo Edge Case (#5543)" This reverts commit 1c2be01. Revert "Backport 4243 to 2.6 (#5509)" This reverts commit 5d90c3b. Revert "[2.6] Fix legacy geofilter leak - [MOD-8568] (#5519)" This reverts commit 221f139. Revert "MOD-7570 Backport 5422 to 2.6 (#5473)" This reverts commit a0f9d4b. Revert "[2.6] MOD-7864: Obfuscation API (#5486)" This reverts commit 11864ea. Revert "[2.6] MOD-7634: Fix NOSTEM (#5484)" This reverts commit 54a82e0. Revert "[2.6] [MOD-8462] rename total_active_writes -> total_active_write_threads (#5445)" This reverts commit 3805366. Revert "Backport info metrics including index error mechanism to 2.6 [MOD-8450] (#5413)" This reverts commit eb8993c. Revert "[2.6] Fixes for inverted indexes encoding - [MOD-8248] (#5390)" This reverts commit 382bc94. Revert "[2.6] MOD-8221: backport info fields (for metrics) to 2.6 (#5375)" This reverts commit 24790ec. Revert "[2.6] Improve "Raw doc id" encoding - [MOD-8255] (#5374)" This reverts commit 778bf80. Revert "[2.6] Improve SkipToBlock logic - [MOD-8255] (#5368)" This reverts commit 775508c. Revert "[2.6][MOD-8115] Free spec resources in the main thread (#5324) (#5331)" This reverts commit fc7b1a6. Revert "[2.6] Fix Sorting Vector - [MOD-6783] (#5315)" This reverts commit 990cdcb. Revert "[2.6] Fix a flakiness in testCursorOnCoordinator - [MOD-8016] (#5306)" This reverts commit 666db12. Revert "[2.6] Fix long PREFIX/SUFFIX/INFIX/WILDCARD queries - [MOD-7882] (#5302)" This reverts commit 2e0da82. Revert "[2.6] Fix missing expansions in text queries - [MOD-8142] (#5286)" This reverts commit a39acfe. Revert "[2.6] MOD-8188: Fix OnReopen Comments (#5289)" This reverts commit 4a08a98. Revert "[2.6] Fix Tag OnReopen Callback - [MOD-8011] (#5281)" This reverts commit ee4d515. Revert "[2.6] Fix MRIterator ownership mechanism - [MOD-8108] (#5270)" This reverts commit 601002a. Revert "[2.6] MOD-8129: Fix indexed_percent info stat (#5250)" This reverts commit 4dc1713. Revert "[2.6] Fix Numeric Tree Balance - [MOD-8081, MOD-8082] (#5201)" This reverts commit b331af9. Revert "[2.6] Performance improvement in indexBulkFields - [MOD-8093] (#5193)" This reverts commit 54c9ded. Revert "[2.6] MOD-8009: Allow Users To Configure Cursor Index Limitation Through Global Config (#5168)" This reverts commit b86fcda. Revert "[2.6] Improve Error Message on VecSim Syntax Error - [MOD-7887] (#5107)" This reverts commit 4c09577. Revert "[2.6] Fix flaky test (#5085)" This reverts commit 8cf0465.

free spec resources in the main thread.

5c3a201

improve cpp gc tests suite:

f6d348e

allow multiple gc runs in each test move args to test suite members use pthread_join instead of pthread_detach loop to enable multiple gc interval taken from #5049

meiravgri added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels Dec 9, 2024

meiravgri requested a review from GuyAv46 December 9, 2024 11:38

GuyAv46 approved these changes Dec 9, 2024

View reviewed changes

GuyAv46 reviewed Dec 9, 2024

View reviewed changes

meiravgri added this pull request to the merge queue Dec 9, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 9, 2024

meiravgri added this pull request to the merge queue Dec 9, 2024

Merged via the queue into master with commit fc50801 Dec 9, 2024

meiravgri deleted the meiravg_fix_cpp_tests_spec_cleanup branch December 9, 2024 15:27

redisearch-backport-pull-request bot mentioned this pull request Dec 9, 2024

[2.10] [MOD-8115] Free spec resources in the main thread #5327

Merged

redisearch-backport-pull-request bot mentioned this pull request Dec 9, 2024

[8.0] [MOD-8115] Free spec resources in the main thread #5328

Merged

meiravgri mentioned this pull request Dec 9, 2024

[2.8][MOD-8115] Free spec resources in the main thread (#5324) #5330

Merged

meiravgri mentioned this pull request Dec 9, 2024

[2.6][MOD-8115] Free spec resources in the main thread (#5324) #5331

Merged

kei-nan added a commit that referenced this pull request Jan 30, 2025

Revert "[2.8][MOD-8115] Free spec resources in the main thread (#5324) (

111997e

#5330)" This reverts commit 5f3ce64.

raz-mon mentioned this pull request Jan 30, 2025

Revert "[2.6] MOD-8601: Fix error message for LOAD (#5531) (#5552)" #5587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MOD-8115] Free spec resources in the main thread#5324

[MOD-8115] Free spec resources in the main thread#5324
meiravgri merged 2 commits intomasterfrom
meiravg_fix_cpp_tests_spec_cleanup

meiravgri commented Dec 9, 2024 •

edited

Loading

Uh oh!

codecov bot commented Dec 9, 2024 •

edited

Loading

Uh oh!

GuyAv46 Dec 9, 2024

Uh oh!

GuyAv46 Dec 9, 2024

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -82,17 +88,16 @@ class FGCTest : public ::testing::Test {
		void runGcThread() {
		fgc = reinterpret_cast<ForkGC *>(get_spec(ism)->gc->gcCtx);

Conversation

meiravgri commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional improvements

Better clean-up of class FGCTest test suite:

Allow multiple gc runs in each test

Uh oh!

codecov bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GuyAv46 Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

GuyAv46 Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

redisearch-backport-pull-request bot commented Dec 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

meiravgri commented Dec 9, 2024 •

edited

Loading

Better clean-up of `class FGCTest` test suite:

codecov bot commented Dec 9, 2024 •

edited

Loading