fix(snapshot): Refactor the big_value_mu_ mutex #4327
Conversation
BagritsevichStepan
force-pushed
the
big-values-serialization/fix-mutex
branch
5 times, most recently
from
f53ae1c to
e784dff
Compare
BagritsevichStepan
force-pushed
the
big-values-serialization/fix-mutex
branch
2 times, most recently
from
3933526 to
2c4e79d
Compare
fixes dragonflydb#4152 Signed-off-by: Stepan Bagritsevich <stefan@dragonflydb.io>
BagritsevichStepan
force-pushed
the
big-values-serialization/fix-mutex
branch
from
2c4e79d to
dc55962
Compare
| // If A.id = 5, and then B.id = 6, and both are blocked here, it means that last_pushed_id_ < 4. | ||
| // Once last_pushed_id_ = 4, A will be unblocked, while B will wait until A finishes pushing and | ||
| // update last_pushed_id_ to 5. | ||
| seq_cond_.wait(lk, [&] { return id == this->last_pushed_id_ + 1; }); |
There was a problem hiding this comment.
I think we don't need this because FlushSerialized is called when big_value_mu_ is locked
There was a problem hiding this comment.
maybe you are right here, but this is not the change I wanted by this PR.
I wanted the mutex to protect the serializer and only it.
So we will have a getter for the serializer when trying to access it and once done we release the mutex.
So in this point the mutex will not be taken
| CHECK(res); | ||
| ++type_freq_map_[*res]; | ||
| } | ||
| } | ||
|
|
||
| size_t SliceSnapshot::FlushSerialized(SerializerBase::FlushState flush_state) { | ||
| io::StringFile sfile; | ||
|
|
||
| // FlushSerialized is already under lock |
There was a problem hiding this comment.
The changes is not good
This is exactly what I wanted to simplify. I dont want us to think or review different flows to understand if the lock is already taken but instead have the getter protect us.
There was a problem hiding this comment.
I suggest revert now all this changes that are for the CallSerializerUnderLock
we can keep the other small changes that you did that does not impact the mutex.
There was a problem hiding this comment.
The issue is that FlushSerialized can be invoked from the flush_fn callback passed to the RdbSerializer, which can be called during the serializer->SaveEntry method (where the mutex is already locked).
There was a problem hiding this comment.
Ok therefore I suggest reverting the changes as they do not simplify the code.
I will revisit this later and see if we can improve this
fixes #4152