spring-data-redis icon indicating copy to clipboard operation
spring-data-redis copied to clipboard

Allow executing scripts inside queueing mode

Open Batanick opened this issue 4 years ago • 4 comments

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?

Batanick avatar Mar 03 '21 11:03 Batanick

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 avatar Mar 03 '21 13:03 mp911de

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 avatar Mar 03 '21 14:03 Batanick

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

Batanick avatar Mar 03 '21 15:03 Batanick

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 avatar Mar 03 '21 15:03 mp911de