DanielLeens commented on PR #11503:
URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5412612844
Thanks for the ping, @SEZ9 — but before answering the substance, a factual
correction, since it changes what actually needs to happen next.
My 2026-08-24 review was **not** cut off. I re-fetched its raw body via the
GitHub API just now: it is 11,710 characters total (well under GitHub's
~65,536-char review-body limit), and the "New finding — a second, distinct,
blocking test failure..." heading you quoted sits at character offset ~2,458 —
near the *start* of the review, not the end. Everything after it, through a
complete `# 5. Merge Recommendation` section with a full closing paragraph, is
present and intact. Nothing was dropped.
The NPE detail you asked me to repost is already there, in full:
- Failing test:
`IncrementalSourceReaderTest.restoreCheckpointStateRestoresTablesAndHistoryFromNewCheckpointFormat`
(`IncrementalSourceReaderTest.java:46-69`)
- NPE site: `IncrementalSourceReader.toCheckpointTablePaths`
(`IncrementalSourceReader.java:319-321`), which calls
`table.getTablePath().getFullName()` with no null guard
- Root cause: the test's `checkpointTables` mock
(`Mockito.mock(CatalogTable.class)`, line 47-48) never stubs `getTablePath()`.
I just re-verified this directly against the current head's source — the mock
genuinely has zero stubbing, and `toCheckpointTablePaths` genuinely
dereferences the result unconditionally. This logging call was introduced in
`b99b1a1b91` (2026-08-22); the test method predates it (`d197162296`,
2026-07-20) — the two had never compiled together successfully until the
`BasicType` typo fix, which is why this NPE was invisible until this round.
- CI evidence: fork run `32638048937` at head `24d4d2e92adf`, `unit-test (8,
ubuntu-latest)`, `unit-test (11, ubuntu-latest)`, `unit-test (8,
windows-latest)` all fail identically.
- Suggested fix: stub `getTablePath()` on that mock (e.g.
`Mockito.when(mockedCatalogTable.getTablePath()).thenReturn(TablePath.of("catalog",
"database", "new_table"));`) before constructing `checkpointTables`. No
production-code change needed.
I also just re-checked the live PR: head is still `24d4d2e92adf` (no new
commits since my review), and the apache-side `Build` check is still failing,
consistent with this NPE being unfixed.
Your F1-F8 items are unrelated to this round's one-line delta and correctly
remain open — no disagreement there. Once the mock is stubbed and CI runs clean
on the corrected head (the Couchbase/DynamoDB IT flakes aside, per my prior
note), I'll do the full pass.
--
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]
