bug(snapshot) : Do not preempt inside OnDbChange issue #829 #868
Conversation
adiholden
commented
Feb 22, 2023
adiholden
commented
- Serializer encode db index of entries
- Do not encode db index inside consume channel
- Allow push serialized data to channel from main snapshot loop and from journal callback (push may preempt)
Signed-off-by: adi_holden <[email protected]>
adiholden
changed the title
src/server/rdb_save.cc
Outdated
| @@ -681,6 +686,7 @@ error_code RdbSerializer::FlushToSink(io::Sink* s) { | |||
| // interrupt point. | |||
| RETURN_ON_ERR(s->Write(bytes)); | |||
| mem_buf_.ConsumeInput(bytes.size()); | |||
| last_entry_db_index_ = kInvalidDbId; // After every flash we should write the DB index again. | |||
There was a problem hiding this comment.
Lets add that we need to do this because the blobs in the channel are interleaved and multiple savers can correspond to a single writer (in case of single file rdb snapshot)
There was a problem hiding this comment.
flash -> flush. (flash is 📸 ).
src/server/rdb_save.cc
Outdated
| // SELECTDB is serialized inside journal writer. In rdb loader when parsing journal blob | ||
| // the journal reader parses the entry db index, and we dont use the main flow of | ||
| // updating the current db index. Unless we change the flow in the loader, we must | ||
| // update last_entry_db_index_ to invalid so that the next snapshot entry to be serialized | ||
| // will update the db index. | ||
| last_entry_db_index_ = kInvalidDbId; |
There was a problem hiding this comment.
So the journal db numbering and rdb db numbering are fully independent, right? Then it seems like we don't have to reset last_entry_db_index_?
| // TODO: investigate why a single byte gets stuck and does not arrive to replica | ||
| for (unsigned i = 10; i > 1; i--) | ||
| CHECK(!default_serializer_->SendFullSyncCut()); | ||
| FlushDefaultBuffer(true); | ||
| CHECK(!serializer_->SendFullSyncCut()); | ||
| PushSerializedToChannel(true); |
There was a problem hiding this comment.
Oh that reminded me we still have this issue 😅
There was a problem hiding this comment.
OMG, maybe we do not? I suggest removing this loop and see if the issue remains.
There was a problem hiding this comment.
We still have bug here
src/server/rdb_save.cc
Outdated
| uint64_t expire_ms) { | ||
| uint64_t expire_ms, DbIndex dbid) { | ||
| SelectDb(dbid); | ||
| last_entry_db_index_ = dbid; |
There was a problem hiding this comment.
is SelectDb used anywhere else?
anyway, I think this line should be moved to SelectDb -line 261
There was a problem hiding this comment.
true this is left overs from my implementation before some fix, now its not relevant. will move inside
src/server/rdb_save.cc
Outdated
| @@ -701,17 +707,20 @@ io::Bytes RdbSerializer::PrepareFlush() { | |||
| return mem_buf_.InputBuffer(); | |||
| } | |||
|
|
|||
| error_code RdbSerializer::WriteJournalEntries(absl::Span<const journal::Entry> entries) { | |||
| error_code RdbSerializer::WriteJournalEntries(const journal::Entry& entry) { | |||
There was a problem hiding this comment.
now it becomes singular WriteJournalEntry
src/server/snapshot.h
Outdated
| // Return if flushed. | ||
| bool FlushDefaultBuffer(bool force); | ||
| // Push serializer's internal buffer to channel. | ||
| // Push regradless of buffer size if force is true. |
Signed-off-by: adi_holden <[email protected]>
Signed-off-by: adi_holden <[email protected]>
|
You didn't add the new FiberAtomicGuard, but we can do it separately when pulling new helio |
