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 evalReadOnly & evalShaReadOnly #880

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented May 30, 2024

These were missing and we will need them :)


Related: https://redis.io/docs/latest/develop/interact/programmability/#read-only-scripts

@BalmungSan
Copy link
Contributor Author

Hi @yisraelU I was wondering if the changes look good to be merged or if you would rather have me change something?

We are needing this at work, so I am happy to help in any way to make a new release including these changes, even if as a temporal SNAPSHOT` just to test them and see if we see an improved performance.

@yisraelU
Copy link
Collaborator

yisraelU commented Jun 5, 2024

Hi @yisraelU I was wondering if the changes look good to be merged or if you would rather have me change something?

We are needing this at work, so I am happy to help in any way to make a new release including these changes, even if as a temporal SNAPSHOT` just to test them and see if we see an improved performance.

Hey @BalmungSan , apologies . I wrote a response last week but I see now it says pending and never got published. 🤷
Here is the gist -
Since the Charset is configurable, I think it would be prefered to add a method here to expose the ClientOptions and use the Charset from the options as opposed to fixing it to UTF_8 (even though UTF_8 is the default , it is configurable)
WDYT ?

@BalmungSan
Copy link
Contributor Author

BalmungSan commented Jun 5, 2024

@yisraelU while I wouldn't mind looking at how to expose that.
You can see that the linked PR in lettuce was merged and will be released as part of 7.0 (although not sure how much time until that).
When that happens, and we upgrade, we can use the overload that accepts a String and that does the encoding using the ChartSet configured by the ClientOps.

IMHO, that is a better solution overall, since we would not duplicate the logic.
Thus, it may be great if we can release a SNAPSHOT from this branch to be usable for us at work, and then when lettuce releases 7.0 we can "fix" this PR and properly merge it.

WDYT?

@yisraelU
Copy link
Collaborator

yisraelU commented Jun 5, 2024

@BalmungSan fair enough . I was originally concerned that 7.0 was a "far off" release, but I can see its the next targeted release.

@yisraelU yisraelU self-assigned this Jun 5, 2024
@yisraelU yisraelU merged commit 81c19b8 into profunktor:series/1.x Jun 5, 2024
2 checks passed
@BalmungSan BalmungSan deleted the add-script-read-only branch June 5, 2024 16:28
@yisraelU
Copy link
Collaborator

yisraelU commented Jun 5, 2024

@BalmungSan seems like snapshots are broken , I am looking into it

@BalmungSan
Copy link
Contributor Author

Thank you very much for the help :)

@yisraelU
Copy link
Collaborator

yisraelU commented Jun 7, 2024

@BalmungSan Snapshots are fixed
Here is the latest version 1.7.0-45-6e6f2c2-SNAPSHOT

@BalmungSan
Copy link
Contributor Author

@yisraelU amazing, thank you very much!
I will notify the team immediately so they can start using it :D

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 this pull request may close these issues.

2 participants