DanielLeens commented on PR #11557: URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-6594498558
Pushed `c5787154a16` (11 commits on top of `41b5818d6b8`) for the blockers in my review of `41b5818d6b8`. One commit per issue. No `pom.xml` change, no deletions. | Issue | Commit | Root cause and fix | Test | |---|---|---|---| | 4 (High) | `fd6d8a49782` | Legacy `pollNext` requeues a custom-row split with the marker `rowNum = -nextIndex-1`, and `addSplits` treated the non-positive `totalRowNum` as a fresh split, so restore re-emitted rows. Negative markers are now kept unchanged; only managed or fresh splits are normalized. | New `FakeSourceReaderTest#legacyCustomRowProgressSurvivesRestore`: restores marker -3 and expects exactly rows 2..5 | | 46 (High) | `7d2b6520170` | The savepoint barrier's `prepareClose` moved the reader to DRAINING before its lifecycle snapshot. `ManagedSourceLifecycle#gracefulCloseForBarrier` now remembers the pre-stop state and `checkpointSnapshot()` persists it. Draining from real end of input happens before the barrier and is still persisted. | `ManagedSourceCheckpointStateTest#shouldResumeRunningAfterRestoringSavepointStop` (savepoint stop restores RUNNING; end-of-input stays DRAINING) | | 47 (High) | `97d4348ce6d` | With checkpoints disabled, `markApplied` removed the entry, so a later admission response failed in `markAdmitted`. The entry is now released only after both facts are consumed, in either order. | `SourceAssignmentTrackerTest#shouldReleaseUncheckpointedAssignmentInEitherResponseOrder` | | 6 (High) | `9d3e844688a` | `releaseAllOutbound` now clears `transportRetries`/`transportFirstAttemptNanos` for every pending and in-flight command of the replaced attempt, so a replay does not inherit the old retry deadline. | None: the coordinator runtime has no unit harness yet | | 10 (High) | `eaaf900b3a6` | `wakeUp` and `interrupt` are now delivered while holding `pollWatchdogLock`. The poll's `finally` clears the generation under the same lock, so delivery cannot outlive the selected poll. | None: needs a reader-runtime harness | | 11 (High) | `32a62989ddd` | The epoch/attempt pair is published as one immutable volatile `CoordinatorFence`, `closed` is volatile, and admission re-checks `closed` inside `admissionLock`. | None: needs a reader-runtime harness | | 7 (High) | `54f0077ef73` | `IcebergSource` reports the legacy capability for bounded (or unconfigured) jobs, so batch Iceberg fails deployment while the feature is on, instead of planning synchronously on the owner loop. The docs table now says streaming only. | None: `IcebergSource` needs a catalog to construct | | 15 (High) | `4a5268fcc39` | Smallest safe option from the review: `ManagedSourceRuntimeSelector#requireOwnedTaskThread` rejects a managed lane at plan time when `task_execution_thread_share_mode=ALL`. Legacy lanes and OFF/PART are unchanged. Documented in EN/ZH. | `ManagedSourceRuntimeSelectorTest#shouldRejectManagedLaneWhenAllTaskThreadsAreShared` | | 26 (Medium) | `fe27c2b44d6` | The managed branch of `SourceFlowLifeCycle#collect` now adds a progressing turn to `SourceReadNanos`, and a turn without progress plus the idle wait to `SourceIdleNanos`, behind the same observability gate as the legacy lane. | None: `SourceFlowLifeCycle` needs a running task | | 3 (Medium) | `39dbff5f543` | Docs fix (option 2 of the review). EN/ZH now explain that every plan rebuild runs selection again and a lane mismatch makes restore fail; they no longer promise pinning. Added an EN/ZH incompatible-changes entry for the lane-bound checkpoint restriction. | Docs only | `c5787154a16` only applies spotless to two of the commits above. **Deferred, because they need an STIP-31 design decision:** - Issue 3, automatic restore pinning. Options: (a) persist the selected lane and capability digest with the job and reuse them when the plan is rebuilt, plus explicit migration rules; (b) keep re-selection, but reject at submission or restore when the flag differs from the checkpoint's lane, giving an early error instead of a task-level restore failure; (c) keep today's fail-closed behavior, which is now documented. - Issue 15, supporting ALL. This needs a serialized logical owner token with an explicit happens-before handoff in place of `checkOwnerThread`, covering reader, coordinator, scheduler and tracker together. **Limits of this push:** - Issues 6, 10, 11, 7 and 26 have no new unit test: the coordinator/reader runtimes, `IcebergSource` and `SourceFlowLifeCycle` have no unit-level harness in this module. Adding one is a separate piece of work. - Locally I ran only `spotless:apply`/`spotless:check` on engine-server, engine-common, connector-fake and connector-iceberg. Compile and tests are left to the fork Build for `c5787154a16`, and I am not claiming them. The obsolete rerun of 41162470910 for the old head was cancelled. - @SEZ9: F1's per-test result for `ManagedSourceRegisterOperationSerializationTest` will come from that new Build's engine-server unit lane. None of these commits touches the operation serializers. -- 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]
