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

scriptLoad command for UnifiedJedis #3151

Closed
yangbodong22011 opened this issue Sep 27, 2022 · 6 comments · Fixed by #3194
Closed

scriptLoad command for UnifiedJedis #3151

yangbodong22011 opened this issue Sep 27, 2022 · 6 comments · Fixed by #3194
Assignees
Milestone

Comments

@yangbodong22011
Copy link
Collaborator

Maybe we shouldn’t split script load and eval/evalsha to add ScriptingControlCommands since they are almost always used together.

@sazzad16
Copy link
Contributor

@yangbodong22011 Few questions to give it more thoughts:

  1. Is your requirement limited only to scriptLoad or all methods in ScriptingControlCommands?
  2. Would your requirement be fulfilled if we implement this only in JedisPooled or must it be in UnifiedJedis?
  3. Does your requirement include pipeline modes as well?
    • If yes, is it only limited to Pipeline class or including all others?

@yangbodong22011
Copy link
Collaborator Author

@yangbodong22011 Few questions to give it more thoughts:

  1. Is your requirement limited only to scriptLoad or all methods in ScriptingControlCommands?

  2. Would your requirement be fulfilled if we implement this only in JedisPooled or must it be in UnifiedJedis?

  3. Does your requirement include pipeline modes as well?

    • If yes, is it only limited to Pipeline class or including all others?
  1. all methods
  2. we implement this only in JedisPooled I didn't understand, it seems that we can only implement in UnifiedJedis. Because JedisPooled extends UnifiedJedis
  3. include pipeline

I think we can just kill ScriptingControlCommands, put all its interfaces in ScriptingKey[Binary]Commands/ScriptingKey[Binary]PipelineCommands and implement it wherever needed.

@sazzad16
Copy link
Contributor

In JedisCluster, what should happen to the methods that are currently part of ScriptingControlCommands? Should we throw UnsupportedOperationException or execute in a random node?

@yangbodong22011
Copy link
Collaborator Author

In JedisCluster, what should happen to the methods that are currently part of ScriptingControlCommands? Should we throw UnsupportedOperationException or execute in a random node?

I think the following is better (although we currently don't have an option for all node execution)
scriptExists random node
scriptLoad all node
scriptFlush all node
scriptKill all node

@d0x7
Copy link

d0x7 commented Nov 7, 2022

Until this is implemented, how would I use SCRIPT LOAD from UnifiedJedis (in my case JedisPooled)? There is only scriptLoad implemented from SampleKeyedCommands, but this requires a second parameter sampleKey. I cannot find any documentation for that, what does sampleKey do? This is the same with every command/method from the SampleKeyedCommands interface.

Overall it feels UnifiedJedis is very incomplete, there's no Pipelining/Multi/Transaction, commands such as PING/INFO/LOLCAT are missing entirely, etc. But I cannot use Jedis because I need the JSON commands which are only implemented in UnifiedJedis.

@sazzad16
Copy link
Contributor

sazzad16 commented Nov 7, 2022

@d0x7

If you are using JedisPooled, you can send any String as sampleKey. Yes, it is not well documented because of low resources and contributions. But the term sampleKey has been in Jedis for some time. So the idea was that if someone was not having issues with it previously, would not have issues afterwards.

To answer the second part of your comment, UnifiedJedis is not meant for connection bound commands like Jedis (class). Pipelining/Multi/Transaction are connection bound features. PING is not really defined if it is not connection bound. INFO is a connection bound command. And I don't recognize LOLCAT. Because of these, those did not make sense to include in UnifiedJedis.

As for JSON commands, those are available in Pipelining/Multi/Transaction. For JSON with other things, please explain [1] why these should co-exist in same class. We can then discuss how to implement it or at least ease the adaptation for users. Thanks

[1] in a separate Issue/Discussion.

@sazzad16 sazzad16 linked a pull request Dec 6, 2022 that will close this issue
@sazzad16 sazzad16 added this to the 4.4.0 milestone Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants