[core] Close every writer and shut both executors down when one close fails - #9227
Open
PDGGK wants to merge 1 commit into
Open
[core] Close every writer and shut both executors down when one close fails#9227PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
… fails
AbstractFileStoreWrite#close walked the writers map with a plain loop:
for (Map<Integer, WriterContainer<T>> bucketWriters : writers.values()) {
for (WriterContainer<T> writerContainer : bucketWriters.values()) {
writerContainer.writer.close();
...
}
}
writers.clear();
if (lazyCompactExecutor != null && closeCompactExecutorWhenLeaving) {
lazyCompactExecutor.shutdownNow();
}
if (lazyPrimaryKeyIndexExecutor != null) {
lazyPrimaryKeyIndexExecutor.shutdownNow();
}
if (compactionMetrics != null) {
compactionMetrics.close();
}
RecordWriter#close is the only call in there that can throw --
BucketedPrimaryKeyIndexMaintainer#close and CompactionMetrics#close are
both declared void with no checked exception. So one writer throwing
takes out everything after it: every remaining writer in the map, the
writers.clear(), both executor shutdowns, and the metrics close. There
is one writer per bucket per partition, so a single bad writer can strand
a large number of them, and the two thread pools stay alive for the rest
of the process.
The writers now go through IOUtils.closeAll, which closes all of them and
rethrows the first failure with the others attached as suppressed. The
tail moves into a finally so it runs whatever the writers did; none of
those four calls throws, so the writer failure is never replaced by one
of them.
Same shape as SortMergeReaderWithMinHeap#close (apache#9163) and the
lookup-store chain (apache#9172), and it uses the helper paimon already has for
it rather than adding another.
Four unit tests. Restoring the plain loop fails three of them -- the
later writers are not closed, the suppressed failure is lost, and the
map is left populated. The fourth covers the path where nothing throws
and passes either way, so the first three are not trivially red.
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
AbstractFileStoreWrite#closewalks the writers map with a plain loop, so the first failure abandons everything behind it:RecordWriter#closeis the only one of these that can throw —BucketedPrimaryKeyIndexMaintainer#closeandCompactionMetrics#closeare both declaredvoidwith no checked exception. So a single writer throwing takes out, in order:writers.clear(),shutdownNow()calls, leaving two thread pools alive for the rest of the process,compactionMetrics.close().The thread pools are the part that outlives the operation.
Tests
Four unit tests in
AbstractFileStoreWriteCloseTest. Restoring the plain loop fails exactly three:master)testCloseReleasesEveryWriterWhenAnEarlierOneThrowstestLaterFailuresRideAlongInsteadOfBeingDroppedtestWriterMapIsClearedEvenWhenAWriterThrowstestCloseSucceedsWhenNoWriterThrowsThe fourth covers the path where nothing throws and passes either way, so the first three are not trivially red.
mvn -pl paimon-core test -Dtest=AbstractFileStoreWriteCloseTest→ 4 run, 0 failures.spotless:checkandcheckstyle:checkonpaimon-coreare clean.API and Format
No change to any public signature, option, or on-disk format. The only behaviour change is on the failure path:
close()still throws the same first exception, but now the later failures are attached to it as suppressed rather than never happening, and the teardown after the loop always runs.The writers go through
IOUtils.closeAll, which is the helper Paimon already has for this — it closes all of them and rethrows the first failure with the rest suppressed. The tail moves into afinallyso it runs whatever the writers did; since none of those four calls throws, the writer's failure is never replaced by one of them.Same shape as
SortMergeReaderWithMinHeap#close(#9163) and the lookup-store chain (#9172).