fix: MONITOR now works for multi transactions #1675
Conversation
Signed-off-by: Vladislav Oleshko <[email protected]>
dranikpg
requested review from
romange and
kostasrim
and removed request for
romange
romange
changed the title
src/server/conn_context.cc
Outdated
| conn_state.db_index = owner->conn_state.db_index; | ||
| conn_state.squashing_info = {owner}; | ||
| } | ||
| delete Inject(crb); // deletes the previous reply builder. |
There was a problem hiding this comment.
I had to look to what Inject does because this statement is raw RAII :D
From my understanding, inject, swaps crb with the internal ReplyBuilder and returns it to the user.
Why don't you add a new operation InjectRelease that instead of returning the old object just releases it via assignment since it's a std::unique_ptr<SinkReplyBuilder> ? That way, we get rid off the raw delete and RAII semantics are handled by the underline unique_ptr
There was a problem hiding this comment.
Actually the better solution would be to pass a flag that the reply_builder is not created at all
src/server/main_service.cc
Outdated
| if (cid->name() == "AUTH") | ||
| return message; | ||
|
|
||
| auto accum = [](auto str, const auto& cmd) { |
There was a problem hiding this comment.
accumulate is a really nice idea here. However it suffers from two issues:
auto stris a deep copy each time the lambda is invoked by the accumulate algorithm. I think only in c++20 theinitstringmessageis moved, by a call tostd::move. So basically now, we are copyingtail_args.size()strings with each of them increasing in size at each step (since we append to the string at each step).- Furthermore, another copy is done when we return
str. This is not one of the cases were NRVO applies, becausestris an input parameter of the lambda.
All in all, I would suggest to rewrite this with a for loop or a for-each.
There was a problem hiding this comment.
True, its code from about a year ago that I just copied over without realizing how problematic it is 😄 (didn't think about checking performance because monitor is not critical)
| print(f"EVAL error: {e}") | ||
| assert False | ||
| collected = await monitor.stop() | ||
| expected = CollectedRedisMsg.all_from_src("SET a 1", "GET a", "LPUSH l V", "LPOP l") |
Signed-off-by: Vladislav Oleshko <[email protected]>
Signed-off-by: Vladislav <[email protected]>
| @@ -123,21 +123,28 @@ struct ConnectionState { | |||
| DflyVersion repl_version = DflyVersion::VER0; | |||
| }; | |||
|
|
|||
| struct SquashingInfo { | |||
| const ConnectionContext* owner = nullptr; | |||
There was a problem hiding this comment.
can you add a comment here why during squashing the owner is not the regular ConnectionContext* that you already have access to?
There was a problem hiding this comment.
Because multiple threads execute commands in parallel during squashing. Added this comment
// Pointer to the original underlying context of the base command.
// Only const access is possible for reading from multiple threads,
// each squashing thread has its own proxy context that contains this info.
Signed-off-by: Vladislav Oleshko <[email protected]>
--bool_flag falseas true)