DanielLeens commented on PR #11494: URL: https://github.com/apache/seatunnel/pull/11494#issuecomment-5419378026
Thanks for the fresh full pass, @SEZ9 — and thanks for continuing to engage on this one. First, on the earlier Issue 1 (possible public API break on `DefaultReader.readData`): this new review doesn't carry it forward, which lines up with the evidence in my 2026-08-05 reply — `readData` is `private` on the current head `bccb2761b`, so I'll take that as resolved unless you want to keep it open explicitly. I re-verified the current head against the eight new findings before replying: - Issue 3 (negative/corrupted `dataLength` not guarded) checks out. `WALDataUtils.byteArrayToInt` does a signed left-shift on the top byte, so a corrupted/torn length can produce a negative `int`. The `dataLength > remainingBytes` check compares an `int` against a `long` and never catches a negative value, so `new byte[dataLength]` throws `NegativeArraySizeException` and aborts the whole recovery instead of skipping a bad tail record — exactly the kind of input WAL recovery has to tolerate. - Issue 6 (`loadAllKeys` retaining full `IMapFileData` incl. value bytes) also checks out. `LatestMutationAccumulator.latestMutations` is `Map<SerializedKey, IMapFileData>`, and `WALReader.loadAllKeys` only reads `mutation.getKey()`/`getKeyClassName()` off it, so peak heap for a key-only listing still scales with total live value bytes rather than key count — that meaningfully undercuts this PR's own memory-optimization claim for that code path. - Issues 1/2/5/7 (serialized-byte vs deserialized-object key identity): the code matches your read — `SerializedKey.equals` is `Arrays.equals` on raw bytes. Worth noting `IMapFileData.key` was already a raw `byte[]` on `dev` before this PR, so the byte-level storage itself isn't new; what's new is using raw-byte identity for dedup/tombstone matching in the accumulator. I agree this is a real, worth-documenting property of the new path, though I'd want to see a concrete non-canonical serializer actually in use in this codebase before calling it a live correctness bug rather than a documented assumption. - Issue 8 (inverted `compareTo` needs a comment): confirmed. `IMapDataComparator.compare` orders the newer record first, so `mutation.compareTo(current) < 0` in the accumulator does mean "mutation is newer" — a one-line comment there is cheap insurance against a future flip. None of these land in a "Blocking items" section this round (there isn't one), so I'm reading this as follow-ups rather than a renewed hold, and my 2026-08-04 approval stands as-is. Issues 3 and 6 look like the most valuable ones for the author to pick up given they touch correctness and the memory-optimization goal directly. -- 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]
