Yicong-Huang commented on PR #6724: URL: https://github.com/apache/texera/pull/6724#issuecomment-5291371369
Retracting the must-fix I posted in round 3 (https://github.com/apache/texera/pull/6724#discussion_r3764810946). It was wrong, and it sent you through a full redesign for the wrong reason. That is on us, not on anything in your implementation. **What I got wrong.** I read `ReconfigurationIntegrationSpec`'s 30s timeout as an engine deadlock and concluded that a born-disabled channel makes a reconfiguration ECM *undeliverable*. It does not. `enable_data()` re-enables every non-control channel in `_queue_ids`, including one that registered mid-pause, so the ECM was **delayed until resume, never lost**. What actually timed out was the test harness, and it was waiting for something production never waits for. **The engine's semantics is "no ECM while paused."** It is stated outright at `ExecutionReconfigurationService.scala:56-57`: > reconfigurations can only come when the workflow is paused, they are not actually performed until the workflow is resumed and the production path is built to match. `ExecutionReconfigurationService.scala:109-111` is `protected def dispatch(request: WorkflowReconfigureRequest): Unit = { client.coordinatorInterface.reconfigureWorkflow(request, ()) }` — the Future is discarded and the method returns `Unit` — and `ExecutionRuntimeService` sends `resumeWorkflow` immediately afterwards. Nothing in production blocks on the reconfigure Future while paused. `TestUtils.scala:375-383` does precisely that: `Await.result(reconfigureWorkflow(...), commandTimeout)` **while still paused**, and only then `resumeWorkflow`. That await is a harness-only contract, and it is what born-disabled violated — not the engine's. **The JVM worker cannot satisfy that contract either.** With `isPaused` true, `DPThread.scala:167-181` has exactly one branch — `tryPickControlChannel`, otherwise `waitingForInput = true` — and that selector filters on `cid.isControl` (`NetworkInputGateway.scala:42`). An ECM sitting in a *data* channel's queue is structurally invisible to a paused Scala worker, and there is no bypass. The pure-Scala `ReconfigurationSpec` stays green only because `mediumCsvScanOpDesc -> keywordSearch` yields a singleton MCS and takes `ReconfigurationHandler.scala:66`'s `scope.size == 1` branch, a direct DCM on the control channel, so it never exercises the ECM branch at all. The contrast inside this very spec makes the point sharper. "modify two python UDFs" builds `source -> udf1 -> udf2` with `targetOps = Seq(udf1, udf2)`, so the Fries component has `scope.size == 2`, takes the ECM branch, and seeds only `sources = {udf1}`. For that test to pass while paused, udf2 must consume an ECM off the udf1->udf2 *data* channel — which `DPThread` would never permit a Scala worker to do. The behavior I asked you to preserve is one no JVM worker has ever had. **Where that leaves the current approach.** The withhold path releases an ECM only when it happens to be a channel's *first* element; when data lands first, the ECM waits for resume anyway — the limitation you documented and pinned with a test. The spec is green because the slow source's first tuple arrives after the pause. Speeding the source up turns the same test red with no code change. So the guarantee ends up timing-dependent rather than structural, and the `put_first`/`enqueue_first` addition to `LinkedBlockingMultiQueue` is what is paying for it. **Suggested direction.** Return to the born-disabled gate from the original revision — it is smaller, leaves `LinkedBlockingMultiQueue` untouched, and matches `DPThread`'s pause semantics — and fix the harness instead: have `TestUtils.shouldReconfigure` follow the production order, dispatching the reconfiguration without awaiting it, then resuming, then awaiting completion. That also removes the timing dependence from the spec. If you see a reason the Python worker *should* diverge from `DPThread` here, push back and I will dig further; I have already been wrong once on this. The round-4 review I posted a few minutes ago stands on its other points (the title, the `put_first` tests, the two comment fixes), but treat anything in it that endorses the withhold design as superseded by this comment. Apologies for the round trip — you implemented exactly what I asked for, and the ask was mine to get right. -- 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]
