Allow memory malloc-stats and memory purge during loading phase
#1317
Conversation
Signed-off-by: Guillaume Koenig <knggk@amazon.com>
knggk
force-pushed
the
memory-commands-during-loading
branch
from
6b826d4
to
09d475d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1317 +/- ##
============================================
+ Coverage 70.73% 70.76% +0.02%
============================================
Files 115 117 +2
Lines 63158 63315 +157
============================================
+ Hits 44674 44802 +128
- Misses 18484 18513 +29
|
|
I would like to suggest only 3 commands except "memory stats" to be allowed during loading process because it costs a lot of resources and involves key issues. (key count, key used memory) |
tests/unit/loading.tcl
Outdated
| $rd ping | ||
|
|
||
| assert_match {Hi Sam, *} [$rd read] | ||
| assert_match {*} [$rd read] |
There was a problem hiding this comment.
Could we do some partial match of string value here?
There was a problem hiding this comment.
Sounds good. I am now matching on *alloc*. It matches both Stats not supported for the current allocator when on Mac, and the actually jemalloc output of interest when on Linux.
tests/unit/loading.tcl
Outdated
| assert_match {Hi Sam, *} [$rd read] | ||
| assert_match {*} [$rd read] | ||
| assert_match {peak.allocated *} [$rd read] | ||
| assert_match {{MEMORY <subcommand> *}} [$rd read] | ||
| # Memory usage keeps getting rejected in loading because the dataset is not visible | ||
| assert_error {*LOADING*} {$rd read} | ||
| assert_match OK [$rd read] | ||
| assert_error {*LOADING*} {$rd read} |
There was a problem hiding this comment.
Could we add comment of the actual command above the response, would be easier to read.
There was a problem hiding this comment.
Agree, have updated as per your recommendation.
Signed-off-by: Guillaume Koenig <knggk@amazon.com>
tests/unit/loading.tcl
Outdated
| @@ -0,0 +1,42 @@ | |||
| start_server [list overrides [list "key-load-delay" 50 loading-process-events-interval-bytes 1024]] { | |||
There was a problem hiding this comment.
The test is failing, i guess we may need a external skip flag.
| start_server [list overrides [list "key-load-delay" 50 loading-process-events-interval-bytes 1024]] { | |
| start_server [list overrides [list "key-load-delay" 50 loading-process-events-interval-bytes 1024] tags [list "external:skip"]] { |
There was a problem hiding this comment.
i commited it so that we can see if the CI will be happy or not.
There was a problem hiding this comment.
It looks like CI is still unhappy but on unrelated tests?
|
please also update the top comment, we can copy the issue content instead of just linking it. |
Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin
added
the
release-notes
knggk
changed the title
knggk
changed the title
memory malloc-stats and memory purge during loading phase
I updated the title and top comment. |
Signed-off-by: Guillaume Koenig <knggk@amazon.com>
Moved the test into introspection.tcl. Let's see if it passes on the CI. |
|
#1368 The expire test is fixed |
memory malloc-statsandmemory purgeare now allowed as they don't depend on the datasetmemory statsandmemory usage keyremain disallowed