[FIXED] Missing tombstones in file store for PurgeEx and Compact
#6685
Conversation
MauriceVanVeen
force-pushed
the
maurice/fs-missing-tombstones
branch
from
b005388 to
fe7579d
Compare
MauriceVanVeen
force-pushed
the
maurice/fs-missing-tombstones
branch
from
fe7579d to
4d38d01
Compare
| @@ -8040,6 +8049,7 @@ func (fs *fileStore) compact(seq uint64, _ /* noMarkers */ bool) (uint64, error) | |||
| // Update fss | |||
| smb.removeSeqPerSubject(sm.subj, mseq) | |||
| fs.removePerSubject(sm.subj, !noMarkers && fs.cfg.SubjectDeleteMarkerTTL > 0) | |||
| tombs = append(tombs, msgId{sm.seq, sm.ts}) | |||
There was a problem hiding this comment.
Should we not do this if we know we are going to nil out below?
We can check dmap or something to know apriori if we will need to write tombstones?
Also maybe allocate the tombs array if we know we will not be re-writing the block.
There was a problem hiding this comment.
Don't think we can know apriori if we'll be allowed to rewrite the block or not? We can check if it's >2MB, but we don't know how many bytes are going to be removed and if we'll drop below half of rbytes as a result.
I've added the append here so we don't need to scan the block twice. Once for calculating bytes, and one for collecting tombs. Now it's just one loop.
What should the tombs array be allocated to?
There was a problem hiding this comment.
We can look at dmap to get an idea no? And current size of course of the block..
There was a problem hiding this comment.
The dmap length for allocation of tombs I think..
There was a problem hiding this comment.
Can't really allocate to the length of dmap since it's not related to the amount of tombstones that need to be written. Any tombstones that do need to be written, are not deleted so are also not part of the dmap.
Think it's best to leave as-is for now, given it's the same implementation as what we already have in PurgeEx as well. Can look at improving performance if needed later.
Signed-off-by: Maurice van Veen <[email protected]>
Signed-off-by: Maurice van Veen <[email protected]>
MauriceVanVeen
force-pushed
the
maurice/fs-missing-tombstones
branch
from
4de0c1a to
1ff9da1
Compare
PurgeExwould not write all tombstones, andCompactwould not write tombstones at all. After a restart all would be good ifindex.dbis available and valid. But if it's missing or invalid, some messages would not be removed due to the missing tombstones, resulting in inconsistencies in the stream state.Also made
rebuildStateLockedconsistent withrecoverMsgs. Must also not set the first sequence to be lower if the message block was empty, and the first sequence was set based on the known last sequence without timestamp.Signed-off-by: Maurice van Veen [email protected]