[format] Safely write NULL for BLOB body fetch failures - #9301
[format] Safely write NULL for BLOB body fetch failures#9301wwj6591812 wants to merge 4 commits into
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
I found two blocking issues in the new staging path. The focused BlobFormatWriterTest suite passes 57/57, but a local counterexample confirms that an InterruptedIOException with a cleared interrupt flag is committed as NULL. The two failing CI jobs are unrelated: Maven Central returned 429, and PrimaryKeyFileStoreTableITCase had an unrelated changelog assertion.
|
|
||
| Throwable current = failure; | ||
| while (current != null) { | ||
| if (current instanceof InterruptedException |
There was a problem hiding this comment.
[P1] Preserve cancellation when InterruptedIOException clears the flag
A plain InterruptedIOException is the standard signal for interrupted I/O, and implementations such as PipedInputStream throw it after the underlying wait has consumed the thread interrupt flag. In that case copyToStaging returns the exception, this method sees neither a set flag nor either listed exception type, and handleSourceReadFailure converts it to NULL when blob-write-null-on-fetch-failure is enabled. The cancelled task can therefore continue and commit a substituted NULL. The existing interruption test misses this because its test stream explicitly re-sets the flag before throwing. I reproduced the cleared-flag case locally: the writer completed and the row read back as NULL. Please preserve cancellation provenance, restore the interrupt, and propagate it. A blanket base-class check needs to exempt timeout subclasses such as SocketTimeoutException, which are intentionally eligible for fallback.
There was a problem hiding this comment.
Thanks, fixed. A plain InterruptedIOException is now recognized at the source-read catch point, so Paimon restores the thread interrupt before source/staging cleanup can replace the original failure. The check matches the exact base class, so timeout subclasses such as SocketTimeoutException remain eligible for the configured fetch fallback. I added regressions for both a cleared interrupt flag and a subsequent source-close failure, and kept the socket-timeout-to-NULL case covered.
|
|
||
| final BlobStaging staging; | ||
| try { | ||
| staging = stagingFactory.create(); |
There was a problem hiding this comment.
[P2] Do not spill already-materialized inline BLOBs
This creates staging for every payload whenever the option is enabled, including exact BlobData values produced by ordinary inline BYTES writes. BlobData already owns a byte array and reads through ByteArraySeekableStream, so there is no remote fetch to make atomic. With the default 1 MiB threshold, every larger inline value is nevertheless copied to java.io.tmpdir, read back, and then written to the final output. Mixed descriptor/inline workloads therefore gain a full extra disk round trip and can fail valid inline rows with local ENOSPC or heavy cross-subtask temp-directory contention solely because a descriptor failure policy is enabled. Please bypass staging for exact BlobData and other provably in-memory sources. For sources that must spill, prefer an engine/task-configured local directory over the process-global default, and cover an inline payload above the default threshold.
There was a problem hiding this comment.
Thanks, fixed. Exact BlobData now bypasses staging, including payloads above the 1 MiB threshold; subclasses remain staged because they may override stream behavior. Descriptor/stream staging now uses the writer task's IOManager temp directory when available, falling back to the process temp directory only when no engine directory exists. I added >1 MiB inline-bypass coverage plus a core-level test that observes a real spill through AppendOnlyWriter -> Dedicated -> Multiple -> BlobFileFormat and verifies cleanup.
|
@JingsongLi hi, please cc, thx |
Purpose
blob-write-null-on-fetch-failure=truecan safely turn a BLOB fetch/open failure into NULL only while no bytes for that BLOB have reached the managed BLOB output. An HTTP-backed BLOB write can also fail later, while the response body is being consumed. One observed failure was:At that point a raw BLOB, ARRAY element, or MAP value may already have appended a record header and a partial payload to the shared BLOB file. Catching the exception and returning NULL directly would leave unindexed bytes behind and could corrupt subsequent offsets, CRCs, and records.
PositionOutputStreamalso has no portable record-level rollback across local, distributed, and object-store implementations.This PR adds a per-BLOB-element staging boundary so the existing opt-in option can safely cover terminal response-body fetch failures.
Changes
When
blob-write-null-on-fetch-failure=true:BlobDatavalues, including inline payloads larger than the spill threshold. Subclasses remain staged because they may override stream behavior.IOManagertemp directory when available; standalone callers without an engine-provided directory fall back to the process temp directory.InterruptedIOExceptionwhose underlying wait cleared the thread flag: restore the interrupt as soon as the source read fails, before cleanup can replace the original failure. Timeout subclasses such asSocketTimeoutExceptionremain eligible for the configured fetch fallback.When the option is false, the existing direct-streaming fast path and failure behavior are unchanged.
Relationship to #9271
This PR and the now-merged #9271 address different layers and are intentionally independent:
#9271 recovers transport interruptions: it uses validated
Range + If-Rangewith a strong ETag, or a complete HTTP 200 replay with SHA-256 prefix verification when no strong ETag is available. It deliberately does not change NULL semantics.This PR supplies the output atomicity needed for the separate terminal policy. With both changes, Paimon first tries bounded body recovery; only after recovery is exhausted does the existing opt-in setting write NULL. #9271 is now merged into master; this PR remains independently reviewable at the format/output layer.
Scope and performance
blob-write-null-on-fetch-failure=true; other writers keep the existing direct path. ExactBlobDatainline values still bypass staging when the option is enabled.Tests
Format-layer unit tests cover scalar, ARRAY, and MAP layouts; good/fail/good sequencing; known-length EOF and unknown-length read errors; option-disabled behavior; reusable-source reopening; exact metrics; cleared-flag cancellation (including a later cleanup failure) versus socket timeout; exact
BlobDatainline bypass above 1 MiB; in-memory and spilled staging; cleanup on success/failure/abort/close; and fatal staging/final-output/consumer failures.Additional focused tests cover
BlobFileFormatTest(44/44),DedicatedFormatRollingFileWriterTest(15/15), the fullAppendOnlyWriter -> Dedicated -> Multiple -> BlobFileFormatIOManager spill path (1/1, including cleanup), and generated configuration documentation completeness (1/1).Flink integration tests use deterministic truncated HTTP responses and cover scalar, ARRAY, and MAP NULL fallback, a following BLOB in the same writer, disabled fallback with no committed row/snapshot, and separation from the 404 option.
Latest master already contains merged #9271. The combined code also passed:
HttpClientUtilsTest: 32 testsThe combined integration coverage verifies both successful recovery staying non-NULL and recovery exhaustion falling back to NULL only when explicitly enabled.
API and format
No existing public API signature or storage-format change.