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 evalLua method in RedisAPI #382

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

liuchong
Copy link
Contributor

Motivation:

Add a convenient method to eval lua script directly, avoid the load-evalsha steps.

Because redis use sha1 to compute script hash, so we can compute it in our client and directly try it wit a evalsha. If failed with a NOSCRIPT error, we can retry with eval and it will cache script, so next time we try evalsha will success.

@liuchong
Copy link
Contributor Author

Please check when you have time @pmlopes @vietj 👀

@pmlopes
Copy link
Member

pmlopes commented May 5, 2023

I'm not sure this is the right way. The client was build with idea of being as close to the protocol as possible so there's just a send and batch to interact, custom APIs should go under the *API wrappers.

The LuaScript probably should be vert.x gen annotated and just have helper method to return a Request instance so you use the current:

redis.send(LuaScript.create("script", args))

To perform hashing it's better to use vertx-auth-common as a single point of codec code:

https://github.com/eclipse-vertx/vertx-auth/blob/master/vertx-auth-common/src/main/java/io/vertx/ext/auth/impl/Codec.java

@pmlopes
Copy link
Member

pmlopes commented May 5, 2023

Also the automatic recovery NOSCRIPT is opionated (and not according to the philosophy of the client) it should belong at a different level of abstraction.

@liuchong
Copy link
Contributor Author

liuchong commented May 6, 2023

Thanks for the comments, here I have some questions or descriptions:

  1. where do you think is better to put these methods?
  2. send(Request) is method of Redis and RedisConnection but not RedisAPI
  3. class LuaScript is mainly for keep the sha1 hash so we do not need to recreate sha1 for every call of evalLua method
  4. NOSCRIPT is beginning string of command EVALSHA, should be ok?
  5. seems we did not added vertx-auth-common dep in this project?

Also, if add evalLua for automatic run eval/evalsha, maybe we could also add evalLuaRo for eval_ro/evalsha_ro.

@pmlopes @tsegismont Could you provide some suggestions? Thanks!

@tsegismont tsegismont self-assigned this May 17, 2023
@tsegismont tsegismont added this to the 4.4.3 milestone May 17, 2023
@vietj vietj modified the milestones: 4.4.3, 4.4.4-SNAPSHOT, 4.4.4 Jun 7, 2023
@tsegismont tsegismont removed this from the 4.4.4 milestone Jun 21, 2023
@tsegismont tsegismont removed their assignment Jun 21, 2023
@tsegismont tsegismont added this to the 5.0.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants