[core] Close the data writer even when the changelog writer fails - #9254
Merged
Conversation
flushWriteBuffer closes both rolling writers in one finally, in sequence. RollingFileWriterImpl.close aborts its own files and rethrows, so a failing changelogWriter.close() skips dataWriter.close() -- and dataWriter is a local that no field, factory hook or MergeTreeWriter.close can reach afterwards, leaving its stream open and its rolled files orphaned. Close both through IOUtils.closeAll, the idiom AbstractFileStoreWrite already uses for the same defect class.
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.
Purpose
MergeTreeWriter#flushWriteBuffercloses two rolling writers in onefinally, in sequence and unguarded:RollingFileWriterImpl#closeaborts its own files and rethrows (RollingFileWriterImpl:181-197), sochangelogWriter.close()genuinely can throw, anddataWriter.close()is then never reached.dataWriteris a local. It appears at its declaration, in theforEach, in thatclose(), and in theresult()loop that runs after thefinally— it is stored in no field and registered nowhere:createRollingMergeTreeFileWriter(KeyValueFileWriterFactory:136-155) does nothing butreturn new RollingFileWriterImpl<>(...).abortManagedBlobWrites()(:125-129), delegates toblobExternalizerand nothing else.MergeTreeWriter#close(:345-384) walksnewFiles,newFilesChangelog,compactAfterandcompactChangelog— all of which are populated after thefinally, so on this path none of them can contain the abandoned writer's output.AbstractFileStoreWrite#close, reachesMergeTreeWriter#close, which holds no reference to the local either.So once the method unwinds, nothing in the process can close or abort that writer: its open stream stays open, and any file it had already rolled is left in the bucket directory unreferenced by any snapshot.
Reachable whenever
changelog-producer = input— that is the only configuration in whichchangelogWriteris non-null.Separately, a
finallythat throws discards the exception in flight, so a genuine failure insidewriteBuffer.forEachis replaced by the close failure rather than carrying it.What changes
Both writers close through the idiom this repo already uses for exactly this, four call sites away in
AbstractFileStoreWrite:381:IOUtils.closeAllskips nulls (IOUtils:199), callsclose()on every element, and rethrows the first failure with the rest attached as suppressed. It throwsException, whichflushWriteBufferalready declares.Blast radius
On the success path both writers are still closed exactly once and in the same order, and
changelogWriter.result()/dataWriter.result()are reached identically. Only the failure path differs.MergeTreeTestBase(both sort engines, 22 tests) passes unchanged.Test
MergeTreeWriterCloseFailureTestdrives aTraceableFileIOwhose streams close and then report failure, which is what a full disk or a rejected object-store finalize looks like. It writes two records through achangelog-producer = inputwriter, expectsprepareCommitto fail, and then asserts that no output stream is left open —TraceableFileIO.openOutputStreams(...), the same checkSingleFileWriterTest:189uses.Reverting to the original
finallyfails it, and names the leak:That is the data writer's stream, still open after the method returned.
MergeTreeWriterCloseFailureTestplusMergeTreeTestBase$MergeTreeTestWithLoserTreeand$MergeTreeTestWithMinHeap: 23 tests, 0 failures.spotless:applyandcheckstyle:checkonpaimon-coreare clean.Note
This is the same defect class as #9227, and the comment that PR left at
AbstractFileStoreWrite:377-380describes it — "closing them in a plain loop meant the first failure abandoned every writer behind it". This is another instance of it, in a method the earlier change did not reach.API and Format
No change to any public signature, option or on-disk format.