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

Implement remaining script API commands #656

Closed
wants to merge 6 commits into from

Conversation

rfmejia
Copy link
Contributor

@rfmejia rfmejia commented Oct 31, 2022

Addresses remaining items in #158.

Please check if tests for these commands are satisfactory, I could only come up with a basic test for scriptDebug, and for scriptKill I couldn't figure out how to fork a long-running script (e.g., infinite loop) and attempt to kill it.

Note: The test only verifies that the "SCRIPT KILL" command is sent to a
live Redis environment by sending a command when no script is running
and tests for an "NOTBUSY" error. Currently it does not test for a
successfully stopped long-running script.
@rfmejia rfmejia requested a review from a team as a code owner October 31, 2022 01:48
@anatolysergeev
Copy link
Collaborator

anatolysergeev commented Nov 12, 2022

Hello, you can try to check ideas from previous try of implementation of this commands.
And there are comments where we decided to postponed their implementation without proper tests.
#269 (comment)
And here you can see my implementation of tests
b68e9d5

May be that can help you

@@ -179,6 +188,28 @@ trait ScriptingSpec extends BaseSpec {
sha <- scriptLoad(lua).either
} yield assert(sha)(isLeft(isSubtype[ProtocolError](hasField("message", _.message, equalTo(error)))))
}
),
suite("scriptFlush")(
test("return true if loaded scripts are flushed from the cache") {
Copy link
Collaborator

@anatolysergeev anatolysergeev Nov 12, 2022

Choose a reason for hiding this comment

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

I think here need check that scripts exists before flush

} yield assertTrue(res == Chunk(false, false))
}
),
suite("scriptKill")(
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are no success test(

@rfmejia
Copy link
Contributor Author

rfmejia commented Nov 24, 2022

Thanks for the review, I checked the linked PR and I tried to perform the same test as you did (infinite loop in Lua and test for scriptKill success) and got the same non-terminating forked fiber issue. I'll investigate this further.

@mijicd
Copy link
Member

mijicd commented Feb 11, 2023

Implemented in #750

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.

3 participants