fix(server): update denyoom flag for all commands #1651
Conversation
| *registry << CI{"JSON.NUMINCRBY", CO::WRITE | CO::DENYOOM | CO::FAST, 4, 1, 1, 1}.HFUNC( | ||
| NumIncrBy); | ||
| *registry << CI{"JSON.NUMMULTBY", CO::WRITE | CO::DENYOOM | CO::FAST, 4, 1, 1, 1}.HFUNC( | ||
| NumMultBy); |
There was a problem hiding this comment.
What's the policy by which we decide on DENYOOM? What is SETBIT allowed and JSON.NUMINCRBY not?
There was a problem hiding this comment.
https://redis.io/commands/json.numincrby/
json.NUMINCRBY increments the already existing field. Besides temporary allocation there is no memory growth here.
| *registry << CI{"JSON.CLEAR", CO::WRITE | CO::DENYOOM | CO::FAST, 3, 1, 1, 1}.HFUNC(Clear); | ||
| *registry << CI{"JSON.ARRPOP", CO::WRITE | CO::DENYOOM | CO::FAST, -3, 1, 1, 1}.HFUNC(ArrPop); | ||
| *registry << CI{"JSON.ARRTRIM", CO::WRITE | CO::DENYOOM | CO::FAST, 5, 1, 1, 1}.HFUNC(ArrTrim); | ||
| *registry << CI{"JSON.CLEAR", CO::WRITE | CO::FAST, 3, 1, 1, 1}.HFUNC(Clear); |
There was a problem hiding this comment.
This should clear memory, right? So why not allow it?
There was a problem hiding this comment.
we do allow it, this is why we removed DENYOOM
| @@ -1315,14 +1315,13 @@ void ListFamily::Register(CommandRegistry* registry) { | |||
| << CI{"LLEN", CO::READONLY | CO::FAST, 2, 1, 1, 1}.HFUNC(LLen) | |||
| << CI{"LPOS", CO::READONLY | CO::FAST, -3, 1, 1, 1}.HFUNC(LPos) | |||
| << CI{"LINDEX", CO::READONLY, 3, 1, 1, 1}.HFUNC(LIndex) | |||
| << CI{"LINSERT", CO::WRITE, 5, 1, 1, 1}.HFUNC(LInsert) | |||
| << CI{"LINSERT", CO::WRITE | CO::DENYOOM, 5, 1, 1, 1}.HFUNC(LInsert) | |||
There was a problem hiding this comment.
This can increase memory usage, right? Why allow it then?
There was a problem hiding this comment.
Reply to all comments above: Its flipped 😄 Deny -> Deny execution in case of oom
There was a problem hiding this comment.
@adiholden please explain in the commit that denyoom flag says which commands can not run in the oom state where flag memory usage crosses the "red zone".
| @@ -1315,14 +1315,13 @@ void ListFamily::Register(CommandRegistry* registry) { | |||
| << CI{"LLEN", CO::READONLY | CO::FAST, 2, 1, 1, 1}.HFUNC(LLen) | |||
| << CI{"LPOS", CO::READONLY | CO::FAST, -3, 1, 1, 1}.HFUNC(LPos) | |||
| << CI{"LINDEX", CO::READONLY, 3, 1, 1, 1}.HFUNC(LIndex) | |||
| << CI{"LINSERT", CO::WRITE, 5, 1, 1, 1}.HFUNC(LInsert) | |||
| << CI{"LINSERT", CO::WRITE | CO::DENYOOM, 5, 1, 1, 1}.HFUNC(LInsert) | |||
There was a problem hiding this comment.
Reply to all comments above: Its flipped 😄 Deny -> Deny execution in case of oom
Signed-off-by: adi_holden <adi@dragonflydb.io>
Signed-off-by: adi_holden <adi@dragonflydb.io>
adiholden
force-pushed
the
update_denyoom
branch
from
f978a37
to
c7d094b
Compare
fixes #1632
DENYOOM command flag marks which commands will be denied when memory usage crosses the "red zone".