SEZ9 commented on PR #11841: URL: https://github.com/apache/seatunnel/pull/11841#issuecomment-6357843425
Quick follow-up on the CI side. The upstream fix for the shared unit-test failure (`LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled` / `InvalidClassException`, seen in [this job](https://github.com/JeremyXin/seatunnel/actions/runs/39534487849/job/119108828964)) has landed in `dev` via `cc54bb433357b4cb45db9cd5d0743589c3111b79`, so there is no longer a reason to wait. This PR's head is still `8ac6d2b4dd7d0d75b9a639664a2e02a4201fa23a` and has diverged from `dev` (ahead 16 / behind 12). Please sync with the latest `dev` and rerun the affected unit checks, plus any stage whose log showed the same archived-wrapper error. Please keep unrelated transform-serialization patches out of this PR — the sync alone should pick up the fix. Note that `all-connectors-it-2`, `all-connectors-it-6`, `hbase-connector-it`, `mysql-cdc-connector-it` and `transform-v2-it-part-2` also failed on that run. The upstream fix does not cover those, so after syncing please take a look at their logs and share whether they are flaky or related to this change. On the source-level review, the previously raised points are still open: - **Legacy payload compatibility (`JobImmutableInformation`, `CoordinatorService.submitJob`)**: payloads with `restoreMode == null` but `isStartWithSavePoint == true` are now treated as non-restore, and `submitJob` derives savepoint-restore only from `RestoreMode`. Both need a fallback to the legacy boolean so savepoint restore from older clients keeps working and SaveMode handling can't wipe data. Related: the null guard on the boolean derivation followed by an unconditional dereference on the next line should be made consistent. - **`ClientJobProxy` wire boolean**: changing its meaning affects old masters in a mixed-version cluster that still branch on it. Please either keep the old semantics on the wire or document the compatibility constraint. - **New `handleSaveMode(SeaTunnelSink, RestoreMode)`**: the restore-aware branch is unreachable since its only call site is guarded by `!jobImmutableInformation.isRestoreJob()`, so CLUSTER-side restore jobs get no schema save-mode handling. Please fix the call site or the guard. - **`CheckpointCoordinator` `isRestoreJob` rename**: no constructor call site is updated in this PR, so the checkpoint-restore semantic change may not actually take effect. Please update the caller(s) and ideally add a test covering checkpoint-mode restore. - **Behavior change for checkpoint-mode restores** hitting a pending cleanup record or existing job metrics (now a `JobException` instead of the savepoint path): if this is intended, please call it out in the PR description / docs so users know what to expect on upgrade. Once the sync is in and the above are addressed, I'll take another pass. Thanks! <!-- streview-comment:1978 --> -- 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]
