Skip to content

Conversation

@zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Feb 7, 2022

There's an assertion added recently to make sure that non-write commands don't use lookupKeyWrite,
It was initially meant to be used only on read-only replicas, but we thought it'll not have enough coverage,
so used it on the masters too.
We now realize that in some cases this can cause issues for modules, so we remove the assert.

Other than that, we also make sure not to force expireIfNeeded on read-only replicas.
even if they somehow run a write command.

See #9572 (comment)

@zuiderkwast zuiderkwast requested a review from oranagra February 7, 2022 08:13
@oranagra oranagra merged commit b571c96 into redis:unstable Feb 8, 2022
@zuiderkwast zuiderkwast deleted the remove-assert-in-lookupKey branch February 8, 2022 15:09
oranagra added a commit that referenced this pull request Apr 18, 2022
…10573)

RM_Yield was missing a call to protectClient to prevent redis from
processing future commands of the yielding client.

Adding tests that fail without this fix.

This would be complicated to solve since nested calls to RM_Call used to
replace the current_client variable with the module temp client.

It looks like it's no longer necessary to do that, since it was added
back in #9890 to solve two issues, both already gone:
1. call to CONFIG SET maxmemory could trigger a module hook calling
   RM_Call. although this specific issue is gone, arguably other hooks
   like keyspace notification, can do the same.
2. an assertion in lookupKey that checks the current command of the
   current client, introduced in #9572 and removed in #10248
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
…edis#10573)

RM_Yield was missing a call to protectClient to prevent redis from
processing future commands of the yielding client.

Adding tests that fail without this fix.

This would be complicated to solve since nested calls to RM_Call used to
replace the current_client variable with the module temp client.

It looks like it's no longer necessary to do that, since it was added
back in redis#9890 to solve two issues, both already gone:
1. call to CONFIG SET maxmemory could trigger a module hook calling
   RM_Call. although this specific issue is gone, arguably other hooks
   like keyspace notification, can do the same.
2. an assertion in lookupKey that checks the current command of the
   current client, introduced in redis#9572 and removed in redis#10248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants