MOD-7795 stop background index when memory is low#4766
MOD-7795 stop background index when memory is low#4766DvirDukhan wants to merge 8 commits intomasterfrom
Conversation
| void IndexSpec_SetIndexErrorMessage(IndexSpec *sp, const char *error, RedisModuleString *key) { | ||
| RedisModule_Assert(sp); | ||
| RedisModule_Assert(error); | ||
| IndexError_AddError(&sp->stats.indexError, error, key); |
There was a problem hiding this comment.
Are these assertions temporary? We will get SEGV if sp is not set.
There was a problem hiding this comment.
if sp is not set, we really messed up
src/spec.c
Outdated
| return; | ||
| } | ||
|
|
||
| RedisModuleServerInfoData *memory_data = RedisModule_GetServerInfo(ctx, "memory"); |
There was a problem hiding this comment.
You can use setMemoryInfo and the globals memoryLimit and used_memory instead of reimplementing the info retrieval again
src/spec.c
Outdated
|
|
||
| RedisModuleServerInfoData *memory_data = RedisModule_GetServerInfo(ctx, "memory"); | ||
| const char *error = NULL; | ||
| if (memory_data) { |
| if(scanner->cancelled) { | ||
| // We need to report the error message besides the log, so we can show it in FT.INFO | ||
| if(scanner->global) { | ||
| // Dvirdu: Do we really get here? |
There was a problem hiding this comment.
I think temporary indexes also use the global scanner
There was a problem hiding this comment.
The only code path I found using global scanner is for legacy index. @MeirShpilraien thoughts?
There was a problem hiding this comment.
Its not legacy indexes, its when we running on Redis version that does not support the load event. I believe this is no longer relevant right?
|
This pull request is stale because it has been open for 60 days with no activity. |
DvirDukhan
force-pushed
the
dvirdu_break_scan_when_low_mem
branch
from
a9d92e0 to
9b57b90
Compare
DvirDukhan
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4766 +/- ##
==========================================
+ Coverage 86.59% 86.69% +0.09%
==========================================
Files 193 193
Lines 34769 34808 +39
==========================================
+ Hits 30109 30177 +68
+ Misses 4660 4631 -29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const char* error; | ||
| // Get the memory limit and memory usage | ||
| setMemoryInfo(ctx); | ||
| if(used_memory > 0.8 * memoryLimit) { |
There was a problem hiding this comment.
Shouldn't we add some search config for that? (default could be 0.8)
| last_indexing_error_key_str: 'doc1', | ||
| last_indexing_error_str: 'Used memory is more than 80%% of max memory, cancelling the scan' | ||
| } | ||
| env.assertEqual(global_index_errors, expected_error_dict) |
There was a problem hiding this comment.
@DvirDukhan We can also make sure the index has no scan in progress now (percent_indexed is 0)
src/spec.c
Outdated
| StrongRef_Release(curr_run_ref); | ||
| } else { | ||
| // spec was deleted | ||
| RedisModule_Log(ctx, "notice", "Scanning index %s in background: cancelled (index deleted)", |
There was a problem hiding this comment.
Is it just how GitHub displays this or is the indentation off here?
|
This pull request is stale because it has been open for 60 days with no activity. |
6 participants
Describe the changes in the pull request
This PR stops background indexing when the memory used is above 80%. The
FT.INFOcommand output will show the memory error as the last indexing error for the index.Which issues this PR fixes
MOD-7795