Skip to content
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

Add Lua global object server as alias for redis #136

Closed
Tracked by #25
zuiderkwast opened this issue Apr 2, 2024 · 5 comments · Fixed by #213
Closed
Tracked by #25

Add Lua global object server as alias for redis #136

zuiderkwast opened this issue Apr 2, 2024 · 5 comments · Fixed by #213
Assignees

Comments

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Apr 2, 2024

We shall also keep redis for backward compatibility.

@zuiderkwast zuiderkwast mentioned this issue Apr 2, 2024
18 tasks
@zuiderkwast zuiderkwast changed the title Add Lua global object server as a alias for redis (which we also keep) Add Lua global object server as alias for redis Apr 2, 2024
@daniel-house
Copy link
Member

Adding a new alias for the word server seems very risky. I can easily imagine that there are already lua scripts with the word server in them. valkey would seem safer.

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Apr 2, 2024

@daniel-house Good point. If script uses a variable named server, they will get an error like I got here:

127.0.0.1:6379> eval 'server = "hello" return server' 0 
(error) ERR user_script:1: Attempt to modify a readonly table script: eaea3495a27b17adb01815dd404912e3f3623d10, on @user_script:1.

@valkey-io/core-team Shall we add a valkey Lua object instead?

I don't think it's a problem actually. Users can only create local variables. It's already possible to create a local variable called redis:

127.0.0.1:6379> eval 'local redis redis = "qux" return redis' 0
"qux"

@madolson
Copy link
Member

madolson commented Apr 2, 2024

Yeah, you have to create local variables. Someone might already be creating a local server that would shadow the global variable, but that shouldn't break their script. It is a valid callout that some folks might need some soft refactoring if they were declaring a local server variable, which would shadow the global one. I don't see a concern.

@madolson madolson self-assigned this Apr 2, 2024
@madolson
Copy link
Member

madolson commented Apr 2, 2024

(Assigning it to me, although someone else from AWS is working on it. I'll update assignment when they join the triage group)

@parthpatel parthpatel self-assigned this Apr 4, 2024
parthpatel added a commit to parthpatel/valkey that referenced this issue Apr 4, 2024
This commit does not remove redis.call/pcall just yet. It also does not rename Redis in error messages such as "Please specify at least one argument for this redis lib call". This allows users to maintain full backwards compatibility while introducing an option to use server.call for new code.

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
parthpatel added a commit to parthpatel/valkey that referenced this issue Apr 4, 2024
This commit does not remove redis.call/pcall just yet. It also does not rename Redis in error messages such as "Please specify at least one argument for this redis lib call". This allows users to maintain full backwards compatibility while introducing an option to use server.call for new code.

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
parthpatel added a commit to parthpatel/valkey that referenced this issue Apr 4, 2024
This commit does not remove redis.call/pcall just yet. It also does not rename Redis in error messages such as "Please specify at least one argument for this redis lib call". This allows users to maintain full backwards compatibility while introducing an option to use server.call for new code.

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
parthpatel added a commit to parthpatel/valkey that referenced this issue Apr 4, 2024
This commit does not remove redis.call/pcall just yet. It also does not rename Redis in error messages such as "Please specify at least one argument for this redis lib call". This allows users to maintain full backwards compatibility while introducing an option to use server.call for new code.

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
@madolson madolson removed their assignment Apr 5, 2024
madolson added a commit that referenced this issue Apr 6, 2024
This commit does not remove redis.call/pcall just yet. It also does not
rename Redis in error messages such as "Please specify at least one
argument for this redis lib call". This allows users to maintain full
backwards compatibility while introducing an option to use server.call
for new code.

I verified that the unit tests pass. Also manually verified that the
redis-server responds to server.call invocations within lua scripting.
Also verified that function registration works as expected.

```
[ok]: EVAL - is Lua able to call Redis API? (0 ms)
[ok]: EVAL - is Lua able to call Server API? (1 ms)
[ok]: EVAL - No arguments to redis.call/pcall is considered an error (0 ms)
[ok]: EVAL - No arguments to server.call/pcall is considered an error (1 ms)
```

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
madolson added a commit to madolson/valkey that referenced this issue Apr 7, 2024
…lkey-io#213)

This commit does not remove redis.call/pcall just yet. It also does not
rename Redis in error messages such as "Please specify at least one
argument for this redis lib call". This allows users to maintain full
backwards compatibility while introducing an option to use server.call
for new code.

I verified that the unit tests pass. Also manually verified that the
redis-server responds to server.call invocations within lua scripting.
Also verified that function registration works as expected.

```
[ok]: EVAL - is Lua able to call Redis API? (0 ms)
[ok]: EVAL - is Lua able to call Server API? (1 ms)
[ok]: EVAL - No arguments to redis.call/pcall is considered an error (0 ms)
[ok]: EVAL - No arguments to server.call/pcall is considered an error (1 ms)
```

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
@zuiderkwast zuiderkwast linked a pull request Apr 10, 2024 that will close this issue
@zuiderkwast
Copy link
Contributor Author

Fixed by #213

PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this issue Apr 24, 2024
…lkey-io#213)

This commit does not remove redis.call/pcall just yet. It also does not
rename Redis in error messages such as "Please specify at least one
argument for this redis lib call". This allows users to maintain full
backwards compatibility while introducing an option to use server.call
for new code.

I verified that the unit tests pass. Also manually verified that the
redis-server responds to server.call invocations within lua scripting.
Also verified that function registration works as expected.

```
[ok]: EVAL - is Lua able to call Redis API? (0 ms)
[ok]: EVAL - is Lua able to call Server API? (1 ms)
[ok]: EVAL - No arguments to redis.call/pcall is considered an error (0 ms)
[ok]: EVAL - No arguments to server.call/pcall is considered an error (1 ms)
```

---------

Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants