DanielLeens commented on PR #11819:
URL: https://github.com/apache/seatunnel/pull/11819#issuecomment-5645082338
Thanks for the confirmation pass, @SEZ9. Responding point by point — I
independently re-traced each against the current head (`922bb392`, unchanged
since my last full re-review) rather than just restating my prior conclusions.
**F7 (stale fingerprint on skip) — confirmed matching your read.**
`MultipleTableFileSourceReaderTest.java` now asserts
`event.getContentFingerprint()` equals the stubbed value on the successful-read
test (line 84) and is `null` on the skip-stale-split test (line 130). That's
exactly the empty/absent-fingerprint assertion you asked me to confirm.
**F8 (`-1`/`0` contract Javadoc) — confirmed.**
`ReadStrategy.getLastReadBytes()` now carries: "`-1` when the strategy does not
report byte progress. Zero means no bytes were consumed and must not advance a
tail split's offset." That's the contract spelled out, matching the consumer
logic in `handleSourceEvent`.
**F1/F2 (post-read check, rows already emitted) — already answered, no new
commit changes this.** This is the same point I addressed in my prior review
under "Where I disagree with @SEZ9 (F1/F6)": `isCurrentTailSplit`'s post-read
re-check intentionally fails the task rather than skip/re-enqueue, because rows
already handed to `output` can't be retracted —
`testCopyTruncateDuringReadFailsBeforeAcknowledgement` locks this in and
asserts `sendSourceEventToEnumerator` is never called on that path. Nothing in
`922bb392` touches this, so my position is unchanged: intentional, not a gap.
**F3 (pre-upgrade restore test) — already answered.**
`testRestoreNormalizesNullInitialTailBaselines` (added in the prior round,
present at this head) nulls both new `FileSourceState` fields via reflection to
simulate a pre-upgrade checkpoint, asserts `readObject` normalizes them to
empty collections, and pins the explicit `serialVersionUID`
(`9208369906513934611L`) so an accidental UID drift would fail CI. That's the
restore-safety test you asked for.
**F4/F5 (inode-reuse documentation vs. stronger identity) — the
documentation half is done; the design half wasn't adopted, and I think that's
the right call to make explicit rather than leave open.** Both `docs/en` and
`docs/zh` now carry the inode-reuse/remount caveat next to the existing Windows
`fileKey()` note (confirmed identical in both languages). On combining
`fileKey` with the content anchor for stronger identity: I don't think this PR
should take that on — it would change `LocalFileIdentity.equals()`/checkpoint
semantics for every existing tailed split, not just document a known
limitation, and the anchor's own 2 KiB-window sampling has the same blind spot
for a same-length in-place rewrite that's already disclosed as Issue 1. I'd
treat F4 as closed by documentation and not fold F5's stronger-identity ask
into this PR.
**F6 — I want to flag something here more precise than a restatement.** Your
framing this round ("only pre-read `NoSuchFileException` is tolerated; other
transient I/O errors on one tailed file will crash-loop the streaming job") is
a narrower and, checking it directly, valid point distinct from the mid-read
case I already ruled on above. Looking at
`MultipleTableFileSourceReader.java:116-140` on the current head: the catch
block's skip condition is `!readStarted && split.getFileIdentity() != null && e
instanceof java.nio.file.NoSuchFileException` — any other exception (a
permission error, an intermittent NFS/network-mount I/O error, etc.) on a
*pre-read* identity/content check falls into the `else` branch, gets wrapped in
`FileConnectorException`, and fails the task. That's a real,
currently-unaddressed gap distinct from the mid-read design decision: a
transient (non-`NoSuchFileException`) I/O error on one file's pre-read check
will fail the whole reader task, and if the same
transient condition persists across a task restart, it would repeat. I'd rate
this Low/non-blocking (same posture as the other open documentation-style
items) rather than a blocker, but it's a legitimate ask for @goutamadwant to
consider widening that catch to cover other `IOException`s the same way, or
documenting why only `NoSuchFileException` is treated as skippable.
**CI status on this exact head.** The apache-side `Build` check shows
`fail`. Tracing to the actual fork run (`34619456595`, on `922bb392`, per this
project's convention that real results live in the fork): it completed with two
job failures, both `updated-modules-integration-test-part-5` shards, both
failing the identical way —
`org.apache.seatunnel.e2e.connector.file.s3.S3FileWithFilterIT` throwing
`ContainerLaunchException`/`ContainerFetchException: Can't get Docker image:
minio/minio:RELEASE.2024-06-13T22-53-53Z`. That's a Docker-image-pull
infrastructure failure in the unrelated S3 connector E2E suite, not the
LocalFile-only diff this PR ships, and not a source-level regression. I'd ask
for those two jobs to be retried rather than treat this as a blocker.
No new commit has landed since I re-reviewed `922bb392`, so this remains a
status/confirmation reply, not a fresh full re-review — my "ready to merge
after CI completes" conclusion stands, pending a clean rerun of the two
image-pull-flake jobs above, plus @goutamadwant's call on the F6
pre-read-exception scope.
--
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]
