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

feat(server): add eval_ro and evalsha_ro #4091

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

andydunstall
Copy link
Contributor

@andydunstall andydunstall commented Nov 9, 2024

Fixes #3880 and #3879

@romange romange requested a review from dranikpg November 9, 2024 06:22
@andydunstall andydunstall marked this pull request as ready for review November 10, 2024 05:54
@adiholden adiholden requested a review from chakaz November 13, 2024 08:52
Copy link
Collaborator

@chakaz chakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution Andy, very clean and elegant! :)
Please see my small comments below

src/server/main_service.cc Outdated Show resolved Hide resolved
src/server/main_service.cc Show resolved Hide resolved
src/server/main_service.h Show resolved Hide resolved
dranikpg
dranikpg previously approved these changes Nov 16, 2024
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean and concise 👍🏻

src/server/main_service.h Show resolved Hide resolved
src/server/main_service.cc Outdated Show resolved Hide resolved
@@ -176,7 +176,8 @@ void Transaction::Shutdown() {
Transaction::Transaction(const CommandId* cid) : cid_{cid} {
InitTxTime();
string_view cmd_name(cid_->name());
if (cmd_name == "EXEC" || cmd_name == "EVAL" || cmd_name == "EVALSHA") {
if (cmd_name == "EXEC" || cmd_name == "EVAL" || cmd_name == "EVAL_RO" || cmd_name == "EVALSHA" ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you checked all places with "EVAL" 🙂 We use some more in main_service.cc, we use abs::StartsWith to be more compact.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix

auto* eval_cid = registry_.Find("EVAL");

this week

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you checked all places with "EVAL" 🙂 We use some more in main_service.cc, we use abs::StartsWith to be more compact.

I think so yeah - the StartsWith ones are ok as will match EVAL_RO? Or do you mean some shouldn't match

src/server/main_service.cc Show resolved Hide resolved
@andydunstall andydunstall merged commit e053639 into dragonflydb:main Nov 24, 2024
9 checks passed
@romange
Copy link
Collaborator

romange commented Nov 24, 2024

Thanks Andy!

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.

implement EVAL_RO
5 participants