DanielLeens commented on PR #11557:
URL: https://github.com/apache/seatunnel/pull/11557#issuecomment-6538141837
Pushed the update promised above. New head: `41b5818d6b8` (two commits on
top of `5dc655e6a91`).
**1. Merged current `dev` (`3ac2d1cec6a`) into the branch** (merge commit
`aa403302293`). Five files conflicted; resolution keeps dev's logic and this
PR's managed-source runtime side by side:
| File | Conflict | Resolution |
|---|---|---|
| `seatunnel-api/.../source/SourceSplitEnumerator.java` | `run()` Javadoc:
#12163 replaced the old "engine guarantees this order" paragraph; this PR had
appended the managed-coordinator note to that paragraph | Kept dev's corrected
guarantee text; the managed note (`isManagedCoordinatorRuntime()` -> return
promptly, use `CoordinatorScheduler`) now follows it as its own `@implNote` |
| `engine-server/.../serializable/TaskDataSerializerHook.java` | dev's
`SOURCE_ENUMERATOR_EVENT_TYPE = 29` (#12163) collided with
`MANAGED_SOURCE_COMMAND_OPERATION = 29` | dev keeps 29; the four managed ids
moved to 30..33 (`MANAGED_SOURCE_COMMAND_OPERATION`,
`MANAGED_SOURCE_REGISTER_OPERATION`, `MANAGED_COORDINATOR_COMMAND_OPERATION`,
`MANAGED_SOURCE_ADMISSION_ACK`). All call sites reference the constants,
nothing hard-codes the numbers; the ids were never released, so no
wire-compatibility impact |
| `engine-server/.../task/SourceSeaTunnelTask.java` | adjacent additions:
dev's `isUnboundedSourceTask()` / `receivedSourceEvent(...)` (#12042, #12163)
vs this PR's `receivedSerializedSourceSplit(...)` | Both kept |
| `engine-server/.../task/context/SeaTunnelSplitEnumeratorContext.java` |
`sendEventToSourceReader`: #12163 turned the legacy no-op into a real
`SourceEnumeratorEventOperation` send; this PR routed it to the managed
transport | Managed coordinator lane -> `task.sendManagedEventToReader(...)`
(unchanged PR behavior); legacy lane -> dev's fire-and-forget operation. Both
import sets kept, and the PR's `getCoordinatorScheduler()` /
`isManagedCoordinatorRuntime()` / `isAssignmentCapacityAvailable()` overrides
follow |
| `engine-server/.../task/flow/SourceFlowLifeCycle.java` | `onTimerTick` and
`triggerBarrier`: #12316 wrapped both in
`SourceCheckpointLockHandoff.injectorArriving()/injectorFinished()` and moved
the flush send under the checkpoint lock; this PR had the managed
`admitFlushSignal` / `admitBarrier` branches and the `sourceRuntimeMetrics`
instrumentation | `triggerBarrier`: dev's handoff wrapper around the lock
block, with the PR's lock-wait / snapshot / barrier-forward metrics inside it,
after the managed early return. `onTimerTick`: managed lane admits the flush
signal to the owner loop; legacy lane uses dev's lock-serialized
`sendFlushSignal` with the `!prepareClose` re-check |
Post-merge audit: `git diff origin/dev --name-status` shows 105 files, no
deletions, no `pom.xml` change, and the same file set as before the merge
(nothing from dev reverted, nothing from this PR dropped).
One point for your judgement rather than something I changed: with a managed
coordinator and a legacy reader (the Iceberg pilot shape),
`ManagedSourceCoordinatorRuntime#sendEventToReader` still returns without
sending ("preserve the legacy context's historical no-op"). After #12163 the
legacy lane itself no longer is a no-op, so that comment's premise is gone.
None of the three pilots sends reader events today, so I left the behavior as
is and did not touch production code in the merge beyond the conflict hunks.
**2. Compile fix for F1** (`41b5818d6b8`):
`SourceAssignmentTrackerTest.java:75` now declares
`Collection<SourceAssignmentTracker.Entry> checkpointEntries =
source.entries();` (plus the `java.util.Collection` import). `restore(...)`
already takes a `Collection`, so no production change. I grepped the module's
tests for other `List<...Entry>` assignments from `entries()`; line 74 was the
only one (the `List` at line 97 comes from `takeAssignmentsForMissingReaders`,
which does return a `List`).
**3. Verification status.** Locally only `spotless:apply` + `spotless:check`
were run, scoped to `seatunnel-api` and `seatunnel-engine-server` (both pass);
compile and tests are left to CI per the validation policy. The fork Build for
`41b5818d6b8` is run
[41162470910](https://github.com/DanielLeens/seatunnel/actions/runs/41162470910)
(queued at the time of writing). The F1 evidence, the per-test result for
`ManagedSourceRegisterOperationSerializationTest`
(`testReaderLocationPayloadRoundTrips`,
`testMismatchedReaderLocationTypeIsRejected`), will come from that run's
`seatunnel-engine-server` unit-test lane; I will post the job link once it has
executed rather than claim it now.
Still outstanding from the list above: the #12747 link in the PR description
has not been added yet in this update.
--
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]
