DanielLeens commented on PR #11988: URL: https://github.com/apache/seatunnel/pull/11988#issuecomment-5793383373
Thanks for the summary, @SEZ9 — I re-checked everything independently against the current head (`2fca2bef97`) rather than taking it at face value, and I don't see anything to push back on. **CI, re-verified directly from the fork run** The apache-side `Build` check is just a pointer, so I pulled the actual fork run for this exact commit: `DanielLeens/seatunnel` run `35357227629`. The only failing jobs are `updated-modules-integration-test-part-2` (JDK 8 and 11) and `paimon-connector-it` (JDK 8 and 11), and all four fail for the same reason: the `minio/minio:RELEASE.2024-06-13T22-53-53Z` image no longer exists on Docker Hub, which breaks the Hudi/Paimon S3 container bootstrap before any test logic runs. None of that touches `connector-cdc-base` or MySQL CDC. Good news on the other side: all four `unit-test` jobs (8/11 x ubuntu/windows) are green, so this branch's own new unit tests (`MySqlIncrementalSourceStartupConfigTest`, `IncrementalSplitAssignerTest`, `IncrementalSourceStreamFetcherTest`, `IncrementalSplitStateTest`, `MixedSplitAssignerTest`, `IncrementalSplitTest`) are actually compiling and passing in CI, not just locally. +1 on the rebase ask. `compare` shows this branch is `diverged` from `dev` (`ahead_by=8`, `behind_by=118`), and current `dev` already carries the `quay.io/minio/minio` mirror fix for those S3 ITs. Rebasing should turn those two jobs green instead of leaving them as something we have to keep explaining away. **Status check on the one open blocker** Nothing about the source has changed since my last full review on this exact head, so I want to be precise about what's carried over vs. new: the missing `mixed`-mode E2E/IT coverage is the same Issue 1 I already raised as blocking, not a new finding. I looked again at `connector-cdc-mysql-e2e` and it already has a directly comparable IT for the `specific` startup mode plus a checkpoint-restore IT, so the ask is concrete: a job with `startup.mode = mixed`, at least one table in `startup.snapshot-table-names` and one outside it, asserting the snapshot table gets its snapshot rows plus subsequent binlog changes, the non-snapshot table only emits changes at or after the configured offset, and both of those hold across a checkpoint stop/restore — since that's exactly where the earlier `pruneTables()` bug lived. The two Low-severity items from before (the restore check being one-directional, and no INFO-level log of the resolved mixed-mode policy) are also unchanged and still non-blo cking. On the checkpoint-compatibility documentation ask: the doc update already states that changing the table set/selection/offset before a restore fails the restore rather than silently changing data coverage, which is a reasonably direct description of what `restoreEnumerator()` actually does. Happy to add a more explicit line about restoring from a checkpoint that was never in `mixed` mode if that's still wanted, but I don't think it's a gap that should hold up the PR on its own. So: same conclusion as last round, now cross-checked against SEZ9's read of the same evidence — the E2E test plus the rebase are what's needed to get this to a clean, mergeable state. I'll do a fresh full pass once both of those land. -- 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]
