SEZ9 commented on PR #12494: URL: https://github.com/apache/seatunnel/pull/12494#issuecomment-6159676970
@Asthenia0412 Yes, please sketch it here first — that's the right call for a change of this size, and I'd rather agree on the shape before you touch more code. A few things I'd like the sketch to cover so we can evaluate it against the findings: - **Generation id end-to-end.** Where the generation id is minted (worker side at deploy, or master side and passed down with the deploy request), how it rides along in `TaskExecutionState` / `NotifyTaskStatusOperation`, and what `updateTaskExecutionState` does with a notification whose generation doesn't match the current one for that `TaskGroupLocation` (drop + log, or drop + metric). This is the core of F1/F2. - **Ordering with `deployTask`.** Even with filtering on the master, I'd like to understand whether the new generation can be deployed at the same location while the old one is still running on the worker, or whether `deployTask` should wait for the old context to actually finish rather than just checking `activeCancellationFuture.isCancelled()`. Filtering alone fixes the master-side attribution; it doesn't by itself prevent two live tasks at one location. - **Compatibility.** Whether the serialized form of `TaskExecutionState` changes and how a master on the old shape handles a worker on the new shape (or vice versa) during a rolling upgrade. Separately from the generation-id work, the remaining items from the earlier review still stand and don't depend on this sketch: - **F3** — `reset()` running the cancellation cascade synchronously under the service monitor, while `deployTask` holds that same monitor across `taskGroup.init()`, `task.init()` and `startedLatch.await()`. Whichever way the generation fix lands, that lock scope should shrink so a merge can't stall behind connector init. - **F4** — `testResetAllowsRedeployOfTaskGroupAtSameLocation` needs to deterministically hit the "is being reset" deploy branch; with `BlockTask` exiting on interrupt the first context is usually gone before redeploy. Once the generation id exists, this test is also the natural place to assert that a stale CANCELED is ignored. - **F5** — the IT resets workers that never left the cluster, so it exercises a failover-like path rather than a real split/merge topology. - **F6** — `SeaTunnelServer.reset()` only resets the worker side on MASTER_AND_WORKER members. If you'd prefer to split the generation-id change into a follow-up PR and keep this one scoped to the reset wiring plus F3–F6, I'm fine with that too — just say so in the sketch and we can decide what the minimum is for this PR to be safe to merge. <!-- streview-comment:1737 --> -- 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]
