CEP-45: Prevent dropping journal segments referenced by partially rec…#4904
CEP-45: Prevent dropping journal segments referenced by partially rec…#4904frankgh wants to merge 1 commit into
Conversation
…onciled sstables Prevent dropping of any segment we might still need in the future to rebuild a partially reconciled sstable, even if every mutation in that segment has been fully reconciled. Segments are droppable once: 1. segment doesn't need replay 2. and no partially reconciled sstables reference any mutations in the segment patch by Francisco Guerrero; reviewed by TBD for CASSANDRA-21406
There was a problem hiding this comment.
Most changes look alright to me. I've left a few small comments in-line. The rest follow below:
- Both migrations to and from tracked will need to be handled. And it's annoyingly nuanced. But I think the naive ref-counting approach won't work. I expect that instead of keeping a single refcount per segment we'll want to keep a set of referrer-SSTables per segment, so that we can have sufficient information to be able to conditionally drop referrer-SSTables from these sets.
- In that vein, we may want to subscribe to SSTable notifications unconditionally on keyspace being tracked. Then base the increment/decrement decision based on an SSTable's
coordinatorLogOffsetsbeing empty or not. LogStatePersister#run()is no longer the logical place to trigger journal truncation. The logical places to trigger it would be: (1) SSTable being flipped from unrepaired to repaired -> refcount reaching0and (2) a segment'sneedsReplay()flipping fromfalsetotrue.- On that note, both of these conditions (
!segment.metadata().needsReplay() && !segmentReferenceTracker.isReferenced(segment.id()) can prevent a segment from becoming droppable for unbounded lengths of time under normal operation. Depending on compaction strategy and on coldness of the coldest table with a reference to a segment, its memtable can take a long time to flush (pre-existing issue) and its SSTables can take a long time to get compacted - and compaction is now where we flip unrepaired to repaired. This latter one is a new issue to this PR, and the solution to it would be to have some process to trigger out of band promotion to repaired (and possibly flush low-traffic long-lived memtables). Maybe not quite a periodic one, but one triggered by some total journal size threshold? - The logic, as implemented, won't quite work for witnesses (including witness-only nodes).
- Lastly, it would be useful to add a column to the mutation journal vtable that lists what sstables are holding segments from being released (or, if we keep refcount as a simple int forever or for now, the SSTable refcount at least).
| lock.lock(); | ||
| try | ||
| { | ||
| return refsBySegment.get(segmentId) > NO_REF; |
There was a problem hiding this comment.
Nit: in count comparisons cleaner and more obvious to just use the literal 0.
| private void decrementRef(long segmentId) | ||
| { | ||
| refsBySegment.compute(segmentId, (k, prev) -> { | ||
| Invariants.require(prev > NO_REF, "Refcount underflow for segment %d", segmentId); |
There was a problem hiding this comment.
Nit (duplicate): in count comparisons cleaner and more obvious to just use the literal 0.
| { | ||
| CommitLogPosition start = startIt.next(); | ||
| CommitLogPosition end = endIt.next(); | ||
| for (long s = start.segmentId; s <= end.segmentId; s++) |
There was a problem hiding this comment.
I think this logic double-counts (or, potentially, n-times-counts) the same segment if multiple non-overlapping intervals are contained in the same segment (but different ranges of position).
Currently it's symmetrical, affecting both increments and decrements in equal measure, so not causing a problem - I think, but I would at least document this, if not fix.
|
And, as more of a note to self rather than a reviewer request, we should eventually add some mutation journal metrics for how long it takes for segments to become truncate-able. |
| private void decrementRef(long segmentId) | ||
| { | ||
| refsBySegment.compute(segmentId, (k, prev) -> { | ||
| Invariants.require(prev > NO_REF, "Refcount underflow for segment %d", segmentId); |
There was a problem hiding this comment.
And one other small thing: if you move out the invariant out of compute() call you won't need a capturing lambda here.
|
One more thing - the tracker probably shouldn't be engaged for SSTables streamed from other hosts (where |
2 participants
…onciled sstables
Prevent dropping of any segment we might still need in the future to rebuild a partially reconciled sstable, even if every mutation in that segment has been fully reconciled.
Segments are droppable once:
patch by Francisco Guerrero; reviewed by TBD for CASSANDRA-21406