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

Detect possible async calls in scripts #1122

Merged
merged 6 commits into from May 1, 2023

Conversation

dranikpg
Copy link
Contributor

Regex for detecting possible async calls for scripts

@dranikpg
Copy link
Contributor Author

dranikpg commented Apr 22, 2023

@romange PTAL at my idea and maybe you spot some bugs or missing cases

I just found one: a multi-line expression can be divided by multiple comment-only lines that we need to exclude.

Multiline comments are also not supported yet

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Contributor Author

I thought maybe we can just detect all comments ahead and remove them - it makes things much easier, however it wouldn't preserve the line numbers for error reporting. Instead of removing then we could replace then with some placeholder, but it doesn't make much sense as the regex still needs to account for them, just in a simper way

src/server/script_mgr.cc Outdated Show resolved Hide resolved
@romange
Copy link
Collaborator

romange commented Apr 23, 2023

The question is whether the frameworks we care about use these hard cases or not.
I think we can be conservative about the cases we want to support, i.e. if it's easier to recognize and skip hard cases then lets do that instead of finding a way to replace them.

@romange
Copy link
Collaborator

romange commented Apr 23, 2023

Another thing I just thought is that it's hard to explain "acall" semantics to users because "asynchronicity" is the implementation detail in Dragonfly. What we really want to say, IMHO, is that a call that explicitly does not return any results is more efficient in Dragonfly.

In that case, should we call it "proc" and not "acall" ?

@dranikpg
Copy link
Contributor Author

In that case, should we call it "proc" and not "acall" ?

Yes, we can call it proc and ppoc(?). I can do a renaming PR afterwards, but lets keep it out of scope here

@romange
Copy link
Collaborator

romange commented Apr 23, 2023 via email

@dranikpg
Copy link
Contributor Author

*pproc - pcall but without reply

@dranikpg
Copy link
Contributor Author

dranikpg commented Apr 23, 2023

Maybe we should enable it only with a flag or atomic modes? See #1091

@romange
Copy link
Collaborator

romange commented Apr 23, 2023 via email

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg marked this pull request as ready for review April 29, 2023 09:29
@dranikpg dranikpg requested a review from romange April 29, 2023 09:29
src/server/multi_test.cc Show resolved Hide resolved
src/server/script_mgr.cc Show resolved Hide resolved
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg dranikpg requested a review from romange May 1, 2023 05:46
@dranikpg dranikpg merged commit 8907222 into dragonflydb:main May 1, 2023
6 checks passed
@dranikpg dranikpg deleted the auto-acall branch May 28, 2023 09:17
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.

None yet

2 participants