DanielLeens commented on PR #11559: URL: https://github.com/apache/seatunnel/pull/11559#issuecomment-6538188308
Follow-up for the [October 26 review](https://github.com/apache/seatunnel/pull/11559#pullrequestreview-5646772958) and the [October 28 conflict note](https://github.com/apache/seatunnel/pull/11559#issuecomment-6473930850). New head: `e7f159373393b9abfa80f479d86d4d27e1091d25` (normal push, no history rewrite; remote SHA read back after the push). **Dev sync (Issue C of the October 24 review).** Merged `dev` at `3ac2d1cec6aa7b3741c4dacdd23020f778e23c02` as a merge commit (`d437f5c376ca2888de8f8f13565d5bb94f135994`). Conflicted files and resolution: - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/flow/SourceFlowLifeCycle.java` (`triggerBarrier`): dev wrapped the checkpoint-lock block in the new `SourceCheckpointLockHandoff` announce/`try`/`finally`, the PR inserted the bootstrap-readiness marker and `snapshotSourceState` inside the same block. Resolution: dev's wrapper encloses the PR's marker `if` and `snapshotSourceState` call; the ack/send lines are dev's re-indented version. - `seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/dag/physical/PhysicalVertex.java`: the PR's `getInputPorts()` and dev's `isRecoverableFinishedStateForRunningPipelineRestore(...)` were added at the same position; both methods are kept, one after the other. - `CheckpointCoordinator`, `CheckpointCoordinatorTest`, `SourceSeaTunnelTask` and `MongodbCDCIT` were changed on both sides but merged without overlapping hunks (dev's additions and the PR's additions are both present; verified hunk-by-hunk against each parent). `IncrementalSourceReader` and `KafkaSourceReader` had no new dev changes since the October 25 sync. Both sides are kept everywhere: dev's logic is unchanged and the PR's additions sit inside it. `git diff --name-status origin/dev` after the merge shows no deleted file and no dev file reverted; the PR's own files are all still present. **Issue 1 (High): `testDynamicLookupBootstrapFailureAndCheckpointRestore` timed out after the engine restart.** Root cause, from code rather than from a rerun: `SeaTunnelContainer` builds every REST URL from `server.getMappedPort(8080)` / `getMappedPort(5801)`, and Testcontainers 1.21.4 `ContainerState.getMappedPort` reads the `InspectContainerResponse` that `GenericContainer` cached at first start (private field, never refreshed). The test restarted the same container with `docker kill` + `docker start`; the engine ports are exposed without fixed host ports, so Docker publishes new ephemeral host ports on that restart. Every poll after `startContainerCmd` therefore went to the host port of the killed process, got connection refused, `ignoreExceptions()` swallowed it, and the two-minute wait at `MysqlCDCCheckpointRestoreIT.java:449-452` could only end in `ConditionTimeout`. The missing second `is STARTED` line in the captured log is the same effect on the log side: the `Slf4jLogCo nsumer` follow stream ends when the container dies and is not re-attached by `docker start`. No other test in `seatunnel-e2e` restarts the engine container in place (`startContainerCmd`/`killContainerCmd` appear only in this test), so there was no existing pattern to reuse. Fix (test infrastructure only, no engine change): - `TestContainer.restartServer()` (new default method, `seatunnel-e2e-common/.../container/TestContainer.java`) and its Zeta implementation `SeaTunnelContainer.restartServer()` (`seatunnel-e2e-common/.../container/seatunnel/SeaTunnelContainer.java:`): starts the stopped container, re-reads the published host ports from a live `inspect`, then waits up to 120 s until (a) the container log contains one more `received new worker register:` line than before the restart, the same readiness signal the initial Testcontainers wait strategy uses, and (b) `GET /running-jobs` on the fresh 8080 port returns 200. The timeout message carries the probe outcome and the last 4000 characters of the server log, so a future failure shows what the new JVM did. - All five REST call sites in `SeaTunnelContainer` (`stopJob`, `getJobStatus`, `getCompletedCheckpointCount`, the post-test `running-jobs` check on 5801 and the thread-leak probe) now go through `mappedPort(int)`, which prefers the bindings read after a restart and falls back to `getMappedPort` otherwise. Without this the shared container would also have stale ports for every later REST-based test in the module, not only for this one. Behaviour for containers that were never restarted is unchanged. - `MysqlCDCCheckpointRestoreIT.java:` replaces `startContainerCmd` plus the `getCompletedCheckpointCount("0") == 0` probe with `container.restartServer()`. The kill, the pre-anchor assertions, the rebootstrap, the exact enriched-row assertions and the checkpoint restore are unchanged; no assertion was removed or loosened and the timeout was not widened. Whether the restarted engine then passes the remaining steps is for CI to show; I did not run the test locally. **Issue 2 (Medium): fact-gate decoder lacked `resolveProxyClass`.** `SourceFlowLifeCycle.SourceGateObjectInputStream` now overrides `resolveProxyClass` and throws `Dynamic lookup fact gate state must not contain proxy classes` (`seatunnel-engine-server/.../task/flow/SourceFlowLifeCycle.java:`), matching `DynamicLookupStateObjectInputStream` and `KafkaGateObjectInputStream`. `SourceFlowLifeCycleGateStateTest` gains `shouldRejectProxyClassesBeforeResolvingTheirHandler` (benign serializable proxy inside a correctly framed, SHA-256-valid envelope, exact message asserted) and `shouldRejectArrayDescriptorsOtherThanByteArray` (empty `String[]` and `HashMap[]` descriptors, exact `Rejected dynamic lookup fact gate state class: [L...;` message). The three existing tests keep their assertions; the envelope framing moved into a shared helper. Removing the new override or widening the array rule flips the asserted exception, so both tests fail on the regression they guard. **Issue 3 (Low):** PR description updated for this head (Kafka gate test rewrite, the test-scoped `connector-kafka` POM dependency, and this follow-up). **Validation.** Per the SeaTunnel local-validation policy only scoped `spotless:apply`/`spotless:check` ran locally (`seatunnel-e2e-common`, `connector-cdc-mysql-e2e`, `seatunnel-engine-server`, plus the modules touched by the merge). Compile, UT, IT and E2E are left to the fork CI run on this head: https://github.com/DanielLeens/seatunnel/actions/runs/41162787674 (Build run 41162787674, queued at the time of writing). No claim of green CI is made for this revision. `pom.xml` files carry only dev's own changes from the merge; no POM conflict occurred and no POM line was edited by hand. -- 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]
