feat(server): Implement SCRIPT FLUSH command #2493
Conversation
src/server/script_mgr.cc
Outdated
| void ScriptMgr::FlushCmd(CmdArgList args, ConnectionContext* cntx) { | ||
| FlushAllScript(); | ||
|
|
||
| auto rb = static_cast<RedisReplyBuilder*>(cntx->reply_builder()); |
There was a problem hiding this comment.
| auto rb = static_cast<RedisReplyBuilder*>(cntx->reply_builder()); | |
| return cntx->SendOk(); |
tests/dragonfly/script_test.py
Outdated
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_script_flush(async_client): |
There was a problem hiding this comment.
We usually use pytest things we can not test in our unit tests.
I think this one is simple to add to our unit tests, check out dragonfly_test.cc for test example on script command tests
tests/dragonfly/script_test.py
Outdated
| assert exists[0] == True | ||
|
|
||
| result = await async_client.script_flush() | ||
| exists = await async_client.script_exists(sha1) |
There was a problem hiding this comment.
In the unit test I would also add a load script after flush and check that it was loaded
src/server/script_mgr.cc
Outdated
|
|
||
| ServerState* ss = ServerState::tlocal(); | ||
| auto interpreter = ss->BorrowInterpreter(); | ||
| interpreter->ResetStack(); |
There was a problem hiding this comment.
add
ss->ReturnInterpreter(interpreter);
| EXPECT_THAT(1, resp.GetInt()); | ||
|
|
||
| resp = Run({"script", "flush"}); | ||
| resp = Run({"script", "exists", sha}); |
There was a problem hiding this comment.
I run you branch and it looks that it does not flushes the scripts.
If you run evalsha sha 0 the script will run and return 5.
Please add this check to unit test and I will try to help to explain what we should do in flush code below
There was a problem hiding this comment.
@adiholden
Thanks for your help!
I add test for evalsha.
494d923
src/server/script_mgr.cc
Outdated
|
|
||
| ServerState* ss = ServerState::tlocal(); | ||
| auto interpreter = ss->BorrowInterpreter(); | ||
| interpreter->ResetStack(); |
There was a problem hiding this comment.
So as I wrote above the ResetStack does not flushes the scripts data from interpreter.
From what I understand we should actually reset all interpreters in all shards.
To do so we need to do
shard_set->pool()->Await([&sha, ¶ms](auto index, auto* pb) {
ServerState::tlocal()->ResetInterpreters();
});
This will make sure that ResetInterpreters will run on all dragonfly shards
ServerState::ResetInterpreters should access interpreter_mgr_ and reconstruct all the Interpreters in InterpreterManager::storage_. also you will need to make sure that no initerperter is borrowed i.e available_.size() == storage_.size() before you reset them as there might be other lua scripts running when you run flush
There was a problem hiding this comment.
@adiholden
Thanks for your help!
I fixed my code according to your advice.
ab2662b
Currently, my code occured segmentation fault when running EVALSHA command after running SCRIPT FLUSH command.
I was investigating this problem.
src/core/interpreter.cc
Outdated
| void InterpreterManager::Reset() { | ||
| waker_.await([this]() { return available_.size() == storage_.size(); }); | ||
|
|
||
| for (auto& interpreter : storage_) { |
There was a problem hiding this comment.
actually I see that we need to reset the storage_ and the available_ as available holds pointers to interperters that are in storage_
check out the InterpreterManager::Get() logic
so you dont need to go over storage_ and call constructor again. reset will be enough
|
Thanks for your contribution! @highpon |
|
@adiholden |
highpon
changed the title
Related Issue: #851