SteNicholas commented on PR #3699: URL: https://github.com/apache/celeborn/pull/3699#issuecomment-4670398240
@saurabhd336 Took another full pass at `c96df79` — re-verified each of my June 8 findings against the current code rather than the commit messages. This has improved a lot: **all four criticals and both highs are genuinely fixed.** ### Verified fixed - **C1 (final-flush failure silently committed)** — `close(boolean)` now throws, propagates compress/fsync/close failures, and only runs `setBytesFlushed`/`replaceFileMeta` on success, with buffer release and channel close in `finally`. A failed trailing flush now fails the commit as it should. - **C2 (per-chunk flags lost on recovery)** — `PbFileInfo` round-trips `chunkCompressed`, and I confirmed graceful-shutdown recovery actually goes through `to/fromPbFileInfoMap` → `PbFileInfo`. (The `PbReduceFileMeta` message I originally pointed at turns out to be dead proto — zero source usages — so this is the right place.) Thanks for extending `PbSerDeUtilsTest`. - **C3** — `DfsPartitionReader.next()` returns `false` unconditionally. - **C4 (decompression authority)** — the read path no longer consults `conf.isChunkCompressionEnabled()` at all; both readers compute `getChunkCompressedCount() > chunkIdx && getChunkCompressed(chunkIdx)`, so an absent list now means **raw**. Exactly the inversion needed — this also kills the version-skew corruption scenarios. - **H1** — `LocalPartitionReader` uses the absolute index; `WorkerPartitionReader` keying off the fetched chunk id is even more robust. **H2** — the sorter guard now throws `IOException`, which `FetchHandler` converts to a clean stream-open failure instead of a hang. - **M1** (`compressBound` + `isError`), **M2** (configurable mmap tmpDir, `MmapMemoryManager.close()` deletes backing files, wired into `StorageManager.close()`), **M4** (level `checkValue`, 0.6.4 version tags) — all good. On the sliced-read thread: I agree with your call. Both the Java and C++ readers always fetch full chunks (offset=0, len=MAX), and if a slice ever does arrive for a compressed chunk, `FileChunkBuffers` throws and `handleChunkFetchRequest` catches it → `ChunkFetchFailure` — clean failure, no hang, no corruption. Fail-fast is right here. ### One new gap found this round **The C++ native client is chunk-compression-blind.** `PbStreamHandler` now carries `chunkCompressed` (field 5), but nothing under `cpp/` reads it, and the C++ `CelebornInputStream` has no ZSTD chunk decompression. A native-engine deployment (Java driver/LifecycleManager + C++ readers) that enables `celeborn.chunk.compression.enabled` would have workers writing ZSTD frames that C++ readers consume as raw shuffle data — batch-codec failures at best, silent garbage if the batch codec is NONE. Ask: document the feature as Java-client-only in the config doc, and ideally make the C++ reader fail fast when any `chunkCompressed` flag is set in the stream handler it receives. That keeps the failure loud instead of silent. ### Non-blocking follow-ups worth tracking (fine post-merge) - mmap buffer pool never shrinks (bounded by peak concurrent writers) and remains invisible to `MemoryManager` congestion control; - AQE/skew reads of compressed files unsupported — now fails cleanly, acceptable for experimental; - `bytesFlushed` uncompressed-then-compressed dual semantics still fragile-but-working. With the C++ gap documented/guarded, I'd consider this mergeable as a default-off experimental feature. Nice work turning the data-safety story around so quickly. -- 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]
