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.

Sign up for GitHub

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

Jump to bottom

Function stats, function kill, fcall and fcall_ro #2486

Merged
merged 20 commits into from
Mar 30, 2023
Merged

Function stats, function kill, fcall and fcall_ro #2486

merged 20 commits into from
Mar 30, 2023

Conversation

elena-kolevska
Copy link
Contributor

No description provided.

@elena-kolevska
Copy link
Contributor Author

Not ready for review yet; I need help with a few things.

@elena-kolevska elena-kolevska self-assigned this Mar 14, 2023
This was linked to issues Mar 14, 2023
Add support for FCALL command #2392
Closed
Add support for FCALL_RO command #2393
Closed
Add support for FUNCTION STATS command #2402
Closed
elena-kolevska
elena-kolevska commented Mar 15, 2023
View reviewed changes
commands_test.go Outdated Show resolved Hide resolved
@elena-kolevska
Copy link
Contributor Author

Let's talk about the API design of FUNCTION STATS in this issue #2402 (comment)

monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
command.go Outdated Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
command.go Outdated Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
commands.go Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
commands.go Outdated Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
commands.go Outdated Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 22, 2023
View reviewed changes
commands.go Outdated Show resolved Hide resolved
@monkey92t
Copy link
Collaborator

@elena-kolevska I have posted some comments, some of which are errors that need to be corrected, some are suggestions, and some are my thoughts on parameter naming.

Also, you need to resolve the conflict in the command.go file :)

monkey92t
monkey92t reviewed Mar 23, 2023
View reviewed changes
command.go Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 23, 2023
View reviewed changes
commands_test.go Outdated Show resolved Hide resolved
monkey92t
monkey92t reviewed Mar 24, 2023
View reviewed changes
command.go Outdated Show resolved Hide resolved
@elena-kolevska elena-kolevska changed the title Function stats, fcall and fcall_ro Function stats, function kill, fcall and fcall_ro Mar 24, 2023
@chayim
Copy link
Contributor

chayim commented Mar 28, 2023

#2398

@elena-kolevska
Copy link
Contributor Author

The test for function stats still doesn't pass, but I tested the code with an external server, where I triggered an FCALL manually, and the proper response is returned.
I repeated the test for Redis Enterprise too.

@elena-kolevska
Copy link
Contributor Author

elena-kolevska commented Mar 29, 2023 •
edited
Loading

@monkey92t @chayim I got it! The problem was that the goroutine was reusing the same connection. I changed it to use its own connection, and now, finally, the test passes :)
Unless there's anything else you think is missing, we can merge this PR.

@elena-kolevska elena-kolevska marked this pull request as ready for review March 29, 2023 15:29
monkey92t
monkey92t approved these changes Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@monkey92t monkey92t left a comment

Choose a reason for hiding this comment

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

Looks very good!

@monkey92t monkey92t merged commit e96c7b5 into redis:master Mar 30, 2023
@elena-kolevska elena-kolevska deleted the function-stats branch March 30, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monkey92t monkey92t monkey92t approved these changes

Assignees

@elena-kolevska elena-kolevska

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for FUNCTION STATS command Add support for FCALL_RO command Add support for FCALL command
3 participants
@elena-kolevska @monkey92t @chayim