-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
E2E MULTI/EXEC/WATCH Support #619
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #619 +/- ##
==========================================
+ Coverage 59.65% 59.91% +0.26%
==========================================
Files 44 44
Lines 15533 15584 +51
Branches 1830 1844 +14
==========================================
+ Hits 9266 9337 +71
+ Misses 6267 6247 -20
|
if (req) { | ||
RedisModule_ReplyWithNull(req->ctx); | ||
} | ||
endClientSession(rr, client_session->client_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should unwatch all keys for this client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment below, unsure its necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take a look at Redis exec implementation? please make sure the we remove the client from the watched clients lists somewhere in our use case.
src/raft.c
Outdated
@@ -412,6 +428,17 @@ RedisModuleCallReply *RaftExecuteCommandArray(RedisRaftCtx *rr, | |||
* (although no harm is done). | |||
*/ | |||
if (i == 0 && cmdlen == 5 && !strncasecmp(cmd, "MULTI", 5)) { | |||
if (client_session) { | |||
uint64_t flags = RedisModule_GetClientFlags(client_session->client); | |||
if (flags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for a specific flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember if we discussed this but why don't we pass multi-exec to rm_call()? So, it can return proper reply depending on client's watched keys? Do we have to manually check dirty result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its possible, currently we don't add "exec" to the log entry, so that would be a change there, was trying to minimize changes needed for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that if we pass watch, multi and exec to the RM_Call(), then it will minimize the API and be quite similar to real clients. Then, we don't have to deal with checking the flag before RM_Call() and no need to generate reply manually.
Also, I assume RM_Call() will reset client after EXEC, so we don't have to destroy moduleclient. Currently, existence of moduleclient is tied to watch/multi/exec, I think it does not have to be like that. We may even maintain a moduleclient per client even they are not watching any keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we can rely on the client being reset after exec. client state persists across exec (just not necc for our session usage).
It might be reasonable to maintain a module client per connected client, but there might be side effects that we dont understand yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly don't want to make this change now, as I think its underestimating the effort and the logical change that we have to the code.
Currently, every time (minus blocking, but that returns a different CallReply type) we do an RM_Call() whether it be within our simulated multi or single commands, it returns a result that we care about.
If we use the persistent client to enqueue, we would get multiple QUEUED responses that we dont care about, and only get a proper response at the very end when we do EXEC().
While this might be an improvement (not convinced, but willing to accept as true), its a fundamental change to a crucial part of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we can rely on the client being reset after exec. client state persists across exec (just not necc for our session usage).
Not sure I understand but I meant dirty flag will be cleaned up automatically, so moduleclient might be reusable. Right now, IIUC you have to free moduleclient once dirty flag is set.
Btw what does necc
mean? :) is it shorthand for necessary?
Ok, indeed queued reply is a bit annoying. Overall, still I see moduleclient as a replacement for real client, so it can carry some state as if like a real connection. Maybe we consider this approach in future if we need something else other than watch. If I'm not mistaken, most changes will be in redisraft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sorry, necc was short for necessary (which I see doesn't make sense as an extra C, should have used nec.
I dont disagree that its possible for the future (though in practice it will just be slower, but perhaps more accurate), as we aren't going to be the queued entries individually on the log, so we will still queue on leader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ClientSession *client_session = NULL; | ||
RedisModule_DictDelC(rr->client_session_dict, &id, sizeof(id), &client_session); | ||
if (client_session) { | ||
freeClientSession(rr, client_session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should call unwatch all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't do anything, it should be no different than calling watch in a normal rm_call(). we are releasing/resetting the client back to the temp pool.
src/raft.c
Outdated
} else { | ||
RedisModule_SetContextUser(ctx, NULL); | ||
} | ||
RedisModule_SetContextClient(ctx, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to do it twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what twice? in one case we set the ctx's user to null, in the other case we set the ctx's client to null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in client_session == True
case, we already set the ctx's client => if we set the ctx's client to null anyway, no need to set it first at client_session
case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we could check it / wrap it in more if/else conditions, but setting it to null always afterwards, even if set up front, seems cleaner.
@@ -376,6 +392,7 @@ RedisModuleCallReply *RaftExecuteCommandArray(RedisRaftCtx *rr, | |||
RedisModuleCallReply *reply = NULL; | |||
RedisModuleUser *user = NULL; | |||
RedisModuleCtx *ctx = req ? req->ctx : rr->ctx; | |||
bool is_multi_session = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is a little bit confusing - it's not a multi session, but a multi session with a valid persist client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in my head this is a multi using session support, not a multi without session support. i.e. is_multi would be if just a multi is multi_session is a multi when we are handling a session.
@@ -574,6 +574,7 @@ static void clientSessionRDBLoad(RedisModuleIO *rdb) | |||
unsigned long long id = RedisModule_LoadUnsigned(rdb); | |||
client_session->client_id = id; | |||
client_session->local = false; | |||
client_session->client = RedisModule_CreateModuleClient(rr->ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about rdbSave? don't we want to save the client state too? (dirty state for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If missing, maybe we can add a test for snapshot & snapshot delivery. Just to hit those lines where we save/restore sessions
src/raft.c
Outdated
} else { | ||
RedisModule_SetContextUser(ctx, NULL); | ||
} | ||
RedisModule_SetContextClient(ctx, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in client_session == True
case, we already set the ctx's client => if we set the ctx's client to null anyway, no need to set it first at client_session
case
if (req) { | ||
RedisModule_ReplyWithNull(req->ctx); | ||
} | ||
endClientSession(rr, client_session->client_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you take a look at Redis exec implementation? please make sure the we remove the client from the watched clients lists somewhere in our use case.
@@ -359,6 +376,16 @@ void handleUnblock(RedisModuleCtx *ctx, RedisModuleCallReply *reply, void *priva | |||
freeBlockedCommand(bc); | |||
} | |||
|
|||
static bool isClientSessionDirty(ClientSession *client_session) | |||
{ | |||
uint64_t flags = RedisModule_GetClientFlags(client_session->client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for a specific dirty flag
src/snapshot.c
Outdated
@@ -707,6 +709,11 @@ static void clientSessionRDBSave(RedisModuleIO *rdb) | |||
ClientSession *client_session; | |||
while (RedisModule_DictNextC(iter, NULL, (void **) &client_session) != NULL) { | |||
RedisModule_SaveUnsigned(rdb, client_session->client_id); | |||
if (RedisModule_GetClientFlags(client_session->client)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment for isClientSessionDirty()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
note - we don't handle discard correctly yet (tearing down session)
src/raft.c
Outdated
@@ -434,17 +471,27 @@ RedisModuleCallReply *RaftExecuteCommandArray(RedisRaftCtx *rr, | |||
* When we have an ACL, we will have a user set on the context, so need "C" | |||
*/ | |||
char *resp_call_fmt; | |||
if (cmds->cmd_flags & CMD_SPEC_MULTI) { | |||
if (cmds->cmd_flags & CMD_SPEC_MULTI || client_session) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know what Redis currently does but can't we send WATCH and then BLPOP (without multi)?
Btw, a test case might be good for this depending on the answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to fix.
Leverages redis/redis#12159 and redis/redis#12219 to enable End to End Multi/Exec/Watch support.
redis/redis#12159 - Enables RedisRaft to have fully implement session support where watches an be tracked and validated for dirtyness. On snapshot we save all current session dirtyness and on restore include that state in the dirtyness check at exec apply time.
redis/redis#12219 - Enables RedisRaft to have finer grain control over command interception. This enables us to handle "PING" and "SHUTDOWN" within a multi correctly. Normally, we don't want to intercept them, but if our client is in a multi state, we do (to prevent shutdown from working, as in normal redis, and to have PING's "PONG" response be part of the MULTI array response).
This PR enables most of the MULTI/EXEC/WATCH tests