fix(amber): wire ExecutionReconfigurationService back to the engine#4531
Merged
Yicong-Huang merged 1 commit intoApr 27, 2026
Merged
Conversation
Contributor
Author
|
Added a smoke test: I deliberately scoped the unit test to that one invariant: the dispatch path that calls Open to refactoring the constructor to take |
zuozhiw
approved these changes
Apr 27, 2026
Contributor
zuozhiw
left a comment
zuozhiw
left a comment
There was a problem hiding this comment.
looks good can we leave a TODO comment to say that the state transfer function for java/scala operators are not implemented
Contributor
Author
yup already added comment |
Yicong-Huang
force-pushed
the
fix/reconfiguration-rpc-wiring
branch
2 times, most recently
from
066d379 to
2cd1167
Compare
Follow-up to apache#4220, restoring the full reconfiguration flow end-to-end. Three coupled gaps: **1. Web-service entrypoint never dispatched.** `ExecutionReconfigurationService.performReconfigurationOnResume` was still throwing `"reconfiguration is tentatively disabled."` and the body that calls `controllerInterface.reconfigureWorkflow` was commented out. Restored, adapted to the current proto shape — `UpdateExecutorRequest` now carries `(targetOpId, newExecInitInfo)` directly, no proto-Any boxing — and resets `ExecutionReconfigurationStore` with a fresh `currentReconfigId` after dispatch. `StateTransferFunc` is dropped because the new request schema doesn't carry it. **2. Engine never reported per-worker completion.** `ReconfigurationHandler` collected the worker `updateExecutor` futures but only returned `EmptyReturn` when they all finished — no `UpdateExecutorCompleted(worker)` events were ever sent to the client. Without those, `ExecutionReconfigurationService.completedReconfigurations` stayed empty, the diff handler never fired, and the frontend never saw `ModifyLogicCompletedEvent`. Each per-worker future is now wrapped with `sendToClient(UpdateExecutorCompleted(worker))` in both Fries-component branches, and the web-service callback is re-enabled. The multi-op branch is also restructured so per-worker futures are added with `++=` instead of the original `+=` that put an `Iterable` into the `ArrayBuffer` as a single element. **3. ECM-embedded ControlInvocation reply was misrouted.** When a `ControlInvocation` travels in-band inside an ECM along a data channel between two workers, the receiving worker was replying along the (swapped) data channel — i.e. back to the upstream worker — rather than to the actor that originated the call (the controller). The upstream worker logged a "received unknown ControlReturn" warning and dropped the reply, so the controller's `Future.collect` on the per-worker futures never resolved. This was masked before because the old multi-op `+=` bug effectively ignored those futures; with #2 above fixing the collection, the routing bug surfaced and hung `ReconfigurationSpec`'s source-propagation test for 2h41m before CI killed it. Both worker implementations now use `command.context.sender / .receiver` — set at invocation time by `mkContext` — instead of the network channel's from/to. For normal RPC over a control channel they're equivalent, so the non-ECM path is unaffected. ReconfigurationSpec's bare `Await.result(...)` calls also gain 1-minute timeouts so future hangs surface in a bounded window instead of running out the GHA 6-hour limit. ### Tests `ExecutionReconfigurationServiceSpec` (new) covers: - empty pending list → no dispatch, store unchanged; - non-empty list → one dispatch carrying the right `(targetOpId, newExecInitInfo)` pairs, store reset with a fresh `currentReconfigId`; - consecutive resumes get distinct `reconfigurationId`s; - worker completion (`onWorkerReconfigured`) updates `completedReconfigurations` with Set semantics (idempotent on duplicates). The test uses three protected seams (`dispatch`, `registerWorkerCompletionCallback`, `registerCompletionDiffHandler`) so the service can be constructed without a live `AmberClient` or `Workflow`. End-to-end engine path is covered by `ReconfigurationSpec` from apache#4220; all five tests including the source-propagation case now pass locally in ~1 minute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Yicong-Huang
force-pushed
the
fix/reconfiguration-rpc-wiring
branch
from
2cd1167 to
8727c03
Compare
Contributor
|
The |
Contributor
Author
|
yeah I am looking into it. it is indeed flaky created an issue too track it #4545 |
This was referenced Apr 28, 2026
Yicong-Huang
added a commit
that referenced
this pull request
Apr 28, 2026
…4546) ### What changes were proposed in this PR? Marks `ReconfigurationSpec`'s `"Engine should propagate reconfiguration through a source operator in workflow"` case as `ignore` to unblock CI. The test consistently hangs at the 1-minute `Await` because the UDF stops making progress after processing the `EndChannel` ECM in the multi-worker (Python source -> Python UDF) propagation path. The other four cases in the spec (single-op python UDF reconfigure, java operator reconfigure, source-as-target rejection, two-UDF chain) still run and pass. A code comment is left at the test pointing at the symptom and the condition for re-enabling. ### Any related issues, documentation, discussions? Put out the fire in #4545. Investigation is in parallel. Follow-up to #4220 (which added the test) and #4531 (which restored the web-service entrypoint but did not fix this hang). ### How was this PR tested? `sbt "WorkflowExecutionService/testOnly *ReconfigurationSpec"` locally — 4 cases pass, the 5th is now skipped (was previously timing out at 1 min). ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (claude-opus-4-7) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Follow-up to #4220, restoring the full reconfiguration flow end-to-end. Two coupled gaps:
1. Web-service entrypoint never dispatched.
ExecutionReconfigurationService.performReconfigurationOnResumewas still throwing"reconfiguration is tentatively disabled."and the body that callscontrollerInterface.reconfigureWorkflowwas commented out. Restored, adapted to the current proto shape (UpdateExecutorRequest(targetOpId, newExecInitInfo)— no more proto-Any boxing). ResetsExecutionReconfigurationStorewith a freshcurrentReconfigIdafter dispatch.StateTransferFuncis dropped — the new request schema doesn't carry it.2. Engine never reported per-worker completion.
ReconfigurationHandlercollected the workerupdateExecutorfutures but only returnedEmptyReturnwhen they all finished — noUpdateExecutorCompleted(worker)events were ever sent to the client. Without those,ExecutionReconfigurationService.completedReconfigurationsstayed empty, the diff handler never fired, and the frontend never sawModifyLogicCompletedEvent. TheUpdateExecutorCompletedcase class was effectively dead code.Each per-worker future is now wrapped with
sendToClient(UpdateExecutorCompleted(worker))in both Fries-component branches (single-op and multi-op). The web-serviceclient.registerCallback[UpdateExecutorCompleted]is re-enabled to advancecompletedReconfigurationson receipt.Any related issues, documentation, discussions?
Follow-up to #4220. See discussion #4016.
How was this PR tested?
ExecutionReconfigurationServiceSpec(new) covers:(targetOpId, newExecInitInfo)pairs, store reset with a freshcurrentReconfigId;reconfigurationIds;onWorkerReconfigured) updatescompletedReconfigurationswith Set semantics (idempotent on duplicates).The test uses three protected seams (
dispatch,registerWorkerCompletionCallback,registerCompletionDiffHandler) so the service can be constructed without a liveAmberClientorWorkflow.End-to-end engine path is covered by
ReconfigurationSpecfrom #4220; the newsendToClientcalls are no-ops when no callback is registered, so existing assertions are unaffected.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-7)