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

Allow executing scripts inside queueing mode #1989

Closed
Batanick opened this issue Mar 3, 2021 · 5 comments
Closed

Allow executing scripts inside queueing mode #1989

Batanick opened this issue Mar 3, 2021 · 5 comments
Labels
status: duplicate A duplicate of another issue

Comments

@Batanick
Copy link

Batanick commented Mar 3, 2021

Hey guys!

Right now it's not allowed to execute Lua scripts from MULTI block, due to check in the JedisScriptingCommands.

As far as I know, this behavior is allowed by Redis

127.0.0.1:6379> MULTI
OK
127.0.0.1:6379> eval "return 42" 0
QUEUED
127.0.0.1:6379> EXEC
1) (integer) 42

and WATCH is also working correctly in such circumstances

127.0.0.1:6379> incr test
(integer) 2
127.0.0.1:6379> watch test
OK
127.0.0.1:6379> multi
OK
# MODIFYING test value from a different client here 
127.0.0.1:6379> eval "return 42" 0
QUEUED
127.0.0.1:6379> exec
(nil)

To give you some context we have a use case when we have a

  • user_data
  • counter
    The user_data that needs to be checked inside app logic (cannot move it to lua :( ), modified and committed together with the counter update

We want to process this in the following manner (pseudo code here):

  1. WATCH user_data
  2. GET user_data
  3. Do some application-level checks and magic
  4. Start MULTI
  5. Execute a script that will decrement the counter and update user_data
  6. Run EXEC

Unfortunately, this is failing right now with

2021-03-03 12:19:28.877 ERROR 36671 --- [    Test worker] c.d.magpie.service.AssignShiftAction     : java.lang.UnsupportedOperationException
	at org.springframework.data.redis.connection.jedis.JedisScriptingCommands.eval(JedisScriptingCommands.java:124)
	at org.springframework.data.redis.connection.DefaultedRedisConnection.eval(DefaultedRedisConnection.java:1523)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at org.springframework.data.redis.core.CloseSuppressingInvocationHandler.invoke(CloseSuppressingInvocationHandler.java:61)

Is this intended behavior or this check can be removed?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 3, 2021
@mp911de
Copy link
Member

mp911de commented Mar 3, 2021

Thanks for bringing up the issue. For quite a while, Jedis' Pipeline and its super-type MultiKeyCommandsPipeline didn't define eval nor script… methods. Seems we missed that Jedis caught up for eval and evalsha. Are you interested in providing a pull request that enables scripting commands for eval and evalSha methods?

@mp911de mp911de added status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 3, 2021
@Batanick
Copy link
Author

Batanick commented Mar 3, 2021

Hey @mp911de , thanks for the quick answer!

As far I understood, the idea is to just remove connection.isQueueing() from JedisScriptingCommands class, right? Or addition refactor is needed?

@Batanick
Copy link
Author

Batanick commented Mar 3, 2021

And btw, connection.isPipelined()is also excessive in this context in this case

@mp911de
Copy link
Member

mp911de commented Mar 3, 2021

We need to apply two changes:

  1. Remove the guards that avoid calling the Jedis methods
  2. Update the call to JedisInvoker to call eval/evalsha on the MultiKeyCommandsPipeline object.

Likely, a few tests rely on when they run in pipelining/transaction mode an exception is thrown.

@mp911de mp911de added status: duplicate A duplicate of another issue and removed status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement labels Sep 11, 2024
@mp911de
Copy link
Member

mp911de commented Sep 11, 2024

Closing as duplicate of #1455

@mp911de mp911de closed this as completed Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

3 participants