ChlineSaurus commented on PR #2904:
URL: https://github.com/apache/jackrabbit-oak/pull/2904#issuecomment-4475353827
What AI flagged (I'm not familiar with this part of the code, so I would
need some time to read up on it, to give a proper review. Since this was
flagged as the mosts risky PR, I'm still adding it):
After a successful compaction (Oak's GC), this PR throws away the entire L2
cache so the new generation of segments can fill it from scratch.
Why it's problematic:
- It's redundant. The cleanup step that runs after every compaction
already clears the cache. This PR adds a second clear earlier in the pipeline.
Net result: two cold-cache windows where there used to
be one. That's strictly worse for hit rate.
- It directly contradicts existing code. Another compaction strategy has
an explicit comment: "we don't clear the segment cache. There might be
transient segments in there." The PR ignores this.
- Wrong order. It clears L2 before the listener that flushes
old-generation segments from the writer cache. In the gap, concurrent commits
can refill L2 with the very old-gen segments we just tried to
evict.
- Partial-success path clears too. Even when compaction only partly
succeeded, it still wipes the cache — including the segments that were just
committed and about to be read.
- Races with concurrent putSegment. Clearing while a writer is mid-insert
can leave L1 references pointing at "unloaded" state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
