davidzollo commented on PR #11271:
URL: https://github.com/apache/seatunnel/pull/11271#issuecomment-5509675025
Thanks for the thorough review. I re-verified each of your 8 findings
against the current head (was ee2a1424199, now 4bd75ee8128 after a fix I'm
pushing as part of this pass) and am leaving this review open (not dismissing)
since I'm addressing part of it myself in this same pass.
**Issue 1 (Blocking) — unguarded live `discoverDataCollections()` call:
RESOLVED**
`IncrementalSourceReader` now has a `discoverCapturedTables()` helper that
wraps the dialect call in try/catch and returns `null` on failure
(IncrementalSourceReader.java:260-270). `pruneRestoredIncrementalSplit()`
returns the split unpruned when `capturedTables == null` (a discovery failure)
or when discovery returned empty while the split still has tableIds
(IncrementalSourceReader.java:272-283), so a transient DB error or
empty/partial discovery result no longer crashes the reader or silently
discards checkpoint state. `addSplits()` also only calls this once per batch
via the `capturedTablesDiscovered` flag (IncrementalSourceReader.java:133-134,
154-157), addressing the "repeated metadata queries" note too. Covered by
`IncrementalSourceReaderTest#testAddSplitsKeepsRestoredSplitWhenDiscoveryFails`,
`#testAddSplitsKeepsRestoredSplitWhenDiscoveryReturnsEmpty`, and
`#testAddSplitsDiscoversCapturedTablesOnlyOncePerBatch`.
**Issue 2 (Medium) — ad-hoc `TableId` reconstruction fragile across
dialects: RESOLVED**
Fixed in commit 7df41cf5d9021a9d00ec05e4f7dda8321ebd80ea (already on head
before this pass): `DataSourceDialect.toTableId(TablePath)` default method +
`Db2Dialect` override for the empty-catalog form, threaded through
`IncrementalSplit.pruneTables(capturedTables, tableIdConverter)` and called as
`dataSourceDialect::toTableId` from the reader. Covered by
`IncrementalSplitTest#testPruneTablesUsesDialectSpecificTableIdConverter` and
`Db2IncrementalSourceFactoryTest`. (This is the same issue nzw921rx raised
concretely for Db2; I dismissed that review with the same evidence.)
**Issue 3 (Medium) — pruning keyed off live discovery, INFO-level
destructive logging: PARTIALLY RESOLVED**
The destructive part is fixed: a transient empty/failed discovery no longer
prunes anything (see Issue 1). Two of your suggestions are still open and I'm
not taking them on in this pass: (a) the empty-split skip log at
IncrementalSourceReader.java:161-165 and the prune-summary log at 287-292 are
still `log.info`, not `log.warn`; (b) pruning still keys off
`discoverDataCollections()` rather than the configured table filter. Leaving
both as non-blocking follow-ups per your own triage.
**Issue 4 (Medium) — missing incompatible-changes.md entry: RESOLVED**
Added to both `docs/en/introduction/concepts/incompatible-changes.md` and
`docs/zh/introduction/concepts/incompatible-changes.md`, describing the
restore-time pruning behavior and the discovery-failure fallback.
**Issue 5 (Medium) — `hasRestoredCheckpointMetadata` heuristic gate: OPEN,
non-blocking**
Unchanged — still infers "restored" from `checkpointDataType != null ||
checkpointTables non-empty || historyTableChanges non-empty`
(IncrementalSourceReader.java:296-302), rather than an explicit restored flag.
I traced `snapshotCheckpointDataType()` (IncrementalSourceReader.java:335-350):
`checkpointTables` is set from
`debeziumDeserializationSchema.getProducedType()` on every checkpoint while in
incremental phase, so in practice this should be non-empty once at least one
record has been produced for the split's tables. Whether it can be empty on an
early checkpoint before any change event has flowed (leaving pruning skipped
for that specific restore) is the kind of restore-timing edge case I don't want
to redesign without being able to run this locally against a real
checkpoint/restore cycle — recommend keeping this open as a dedicated follow-up
rather than guessing at a fix here.
**Issue 6 (Medium) — `pruneTables()` unguarded
`tableIds`/`completedSnapshotSplitInfos`: FIXED IN THIS PASS**
Just pushed commit 4bd75ee8128903c57c647b5259bb6725e1509c6a: both are now
null-guarded the same way `checkpointTables`/`historyTableChanges` already
were, falling back to an empty list instead of NPEing. Added
`IncrementalSplitTest#testPruneTablesToleratesNullTableIdsAndCompletedSnapshotSplitInfos`
to cover it.
**Issue 7 (Low) — thin Javadoc: RESOLVED**
`pruneTables()` now has a full Javadoc with `@param`/`@return` and an
explanation of the `TablePath`/`TableId` namespace mismatch this method bridges
(IncrementalSplit.java:141-151).
**Issue 8 (Low) — log message casing/verbosity: OPEN, non-blocking**
Unchanged — the skip message still starts lowercase ("subtask {} skip ...",
IncrementalSourceReader.java:162-165) and the prune-summary log still dumps
full before/after tableId lists (IncrementalSourceReader.java:287-292). Leaving
as a non-blocking style nit per your own triage.
Summary: your sole Blocking item (Issue 1) and the two other substantive
correctness issues (2, 6) are resolved with the evidence above; docs (4) and
Javadoc (7) are done. Issues 3 and 8 have partial/no action (logging nits,
explicitly non-blocking in your own conclusion). Issue 5 is a real open
question I'm flagging rather than guessing at, since I have no way to verify a
redesign of the restore-detection gate without running this against a live
checkpoint/restore cycle. Given your own conclusion was "can merge after the
blocking items are fixed" and Issue 1 is now resolved, I'd appreciate you
taking another pass at the current head (4bd75ee8128) when you have a chance —
I'm leaving this review open rather than dismissing it myself since I just
pushed a fix touching part of it in this same 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]
