QUEUE-130: Release parked store after StoreAppender construction#1714
Open
james-mcsherry wants to merge 5 commits into
Open
QUEUE-130: Release parked store after StoreAppender construction#1714james-mcsherry wants to merge 5 commits into
james-mcsherry wants to merge 5 commits into
Conversation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



QUEUE-130: Release parked store after StoreAppender construction
Issue
When a
StoreAppenderis constructed, it runs an EOF back-scan that walks cycles from the last existing cycle down to the first, and parks the appender on a store (the newest EOF-ed cycle, or the first cycle). This parked store — its mapped-file reservation, wires, and open file descriptor — was held open even though the appender had not written anything yet.An appender created against a queue that already has older roll cycles therefore pinned an old
.cq4file open. BecauseFileUtil.removableRollFileCandidates(...)scans oldest-first and stops at the first still-open file, a single pinned old file blocked removal of that file and every file after it. A parked appender that never wrote could thus prevent old roll files from being cleaned up.Fix
StoreAppendernow calls a newreleaseParkedStore()at the end of the back-scan. This drops the store, its wires, and the FD, returning the appender to the same "never written" state as one created against an empty queue. The next write re-acquires the current cycle as normal. Nothing on disk is changed — only the in-memory reservation and open descriptor are released.A
scannedCyclelocal was added so the "took Nms to find first open cycle" perf log still reports the cycle the scan landed on, sincecycleis now reset by the release.New test
AppenderReleasesParkedStoreTest(extendsIndexingTestCommon) proves the behaviour at the symptom level viaFileUtil.removableRollFileCandidates(...):writerAloneLeavesEveryEarlierCycleRemovable— control: with only the writer active, all earlier cycles are removable.parkedAppenderDoesNotPinAnOldCycle— a fresh appender parks at construction and never writes; after the writer rolls past it, every earlier cycle must still be removable.removableRollFileCandidatesinspects open descriptors via/proc/self/fd, so both tests are gated ongetAllOpenFilesIsSupportedOnOS()and skip on platforms where that is unsupported (macOS/Windows). Commenting out thereleaseParkedStore()call turnsparkedAppenderDoesNotPinAnOldCyclered while the control stays green.Why existing tests needed changing
CreateAtIndexTestandNotCompleteTestcompare queue dumps before/after an append. With the fix, the appender's first write now performs EOF normalisation, which writes a benignnormalisedEOFsTometadata record and shifts byte/modCountcounters. These are expected, harmless deltas, not behavioural changes, so the dump-comparison helpers were extended to normalise them out:normalisedEOFsTometadata record.modCountvalues.# N bytes remainingcounts.IndexingTestCommon.timeProviderwas madeprotectedso the new test in the same package hierarchy can advance time to force rolls.