RED-134847 support for active defrag for RedisJSON. #1262
Conversation
The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * that increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionaty will not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1262 +/- ##
==========================================
- Coverage 78.44% 78.37% -0.07%
==========================================
Files 14 15 +1
Lines 3938 4004 +66
==========================================
+ Hits 3089 3138 +49
- Misses 849 866 +17 ☔ View full report in Codecov by Sentry. |
| env.cmd('CONFIG', 'SET', 'active-defrag-cycle-min', '99') | ||
|
|
||
| try: | ||
| env.cmd('CONFIG', 'SET', 'activedefrag', 'yes') |
There was a problem hiding this comment.
@MeirShpilraien i understand that unlike the redis defrag tests, this one isn't actually creating fragmentation and waiting for it to go down, which is ok.
and i understand that it does test all sorts of various types of json objects, and watches for stats about callback cycles, but how does it make sure that the pointers were really moved? what if redis forgets to call the module api callback?
maybe in addition to these, add one test that create holes and waits for them to get filled?
There was a problem hiding this comment.
Modify to tests to actually cause fragmentation and verify that defrag actually took place.
There was a problem hiding this comment.
if i read it correctly, you created a lot of jason fields, and then deleted all of them.
what i think you should do:
- disable active defrag
- create a lot of jason fields
- delete all the even ones (or alternatively when you create them, you can create two keys, and interchangeably add fields to both, and then delete one key)
- verify you managed to create fragmentation beyond a certain threshold.
- enable active defrag.
- wait for fragmentation levels to go down
see memefficiency.tcl in redis.
what i wanna see is that fields get actually re-located (not just that the callback is called)
tests/pytest/test_defrag.py
Outdated
| # wait for fragmentation for go down for up to 30 seconds | ||
| frag = env.cmd('info', 'memory')['mem_fragmentation_ratio'] | ||
| startTime = time.time() | ||
| while frag < 1.1: |
There was a problem hiding this comment.
shouldn't it be >?
p.s. i think allocator_frag_ratio is safer to use.
There was a problem hiding this comment.
shouldn't it be >?
Don't we want the fragmentation ratio to go down?
p.s. i think allocator_frag_ratio is safer to use.
OK, will change it.
There was a problem hiding this comment.
shouldn't it be >?
Ohh sorry right, and when I used the allocator_frag_ratio it actually failed (went down to fast?)
There was a problem hiding this comment.
maybe.. seems odd, but no reason to wonder about it too much
| @@ -110,12 +110,12 @@ def testDefragBigJsons(env): | |||
| # wait for fragmentation for up to 30 seconds | |||
There was a problem hiding this comment.
you can say that stats are updated by cron (the fragmentation is already a fact, we just wait for it to be detected).
one second (or even 200ms) is enough, no need for 30 sec.
There was a problem hiding this comment.
@oranagra we will not wait for 30 seconds, we will wait for up to 30 seconds to see fragmentation and if we do not see it by than we will fail the test.
There was a problem hiding this comment.
i know.. still saying that there's no need.
IMHO it's misleading, we're just waiting for cron to update stats..
anyway, i don't really care.
The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * Increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality. (cherry picked from commit 6539e1e)
The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * Increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality. (cherry picked from commit 6539e1e)
The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred: * [Redis defrad module API extentions](redis/redis#13509) * [redismodule-rs support for active defrag API](RedisLabsModules/redismodule-rs#387) * [IJSON support for defrag](RedisJSON/ijson#1) The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key. **Notice**: * Increamental defrag of the json key is **not** support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment. * If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged. Tests were added to cover the new functionality. (cherry picked from commit 6539e1e)
The PR adds support for active defrag on RedisJSON. A pre condition for this PR is that the following PR's will be megred:
The PR register defrag function on the json datatype and uses the new capability of ISON to defrag the key.
Notice:
Increamental defrag of the json key is not support. In order to support it we need to implement the free_effort callback. This is not trivial and even if it was it would have cause the json object to potentially be freed when the GIL is not hold, which violate the assumption of our shared string implementation (it is not thread safe). We leave it for future improvment.
If we run on a Redis version that do not support the defrag start callback, we can still partially support defrag. In that case the IJSON object will be defraged but the shared strings dictionatrywill not be reinitialize. This basically means that shared strings will not be defraged.
Tests were added to cover the new functionality.