You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "Yicong-Huang (via GitHub)" <gi...@apache.org> on 2026/04/26 18:46:20 UTC
[PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Yicong-Huang opened a new pull request, #4531:
URL: https://github.com/apache/texera/pull/4531
### What changes were proposed in this PR?
Follow-up to #4220 (which re-enabled reconfiguration on the engine side per discussion #4016).
The engine-side wiring is in place — `ReconfigurationHandler.reconfigureWorkflow` runs the Fries algorithm, `UpdateExecutorHandler` applies `UpdateExecutorRequest` on workers, the proto RPC `ControllerService.ReconfigureWorkflow` is registered. But the web-service entrypoint at `ExecutionReconfigurationService.performReconfigurationOnResume` was left in its previously-disabled state:
```scala
throw new RuntimeException("reconfiguration is tentatively disabled.")
// (the rest of the method, including the actual reconfigureWorkflow RPC call, was commented out)
```
This means any user who pauses a running workflow, modifies an operator's properties, and resumes — exactly the user-facing flow `modifyOperatorLogic` was designed for — gets a `RuntimeException` thrown on resume.
This PR restores the previously-commented-out body, adapted to the current proto/RPC shape:
- `UpdateExecutorRequest` is now `(targetOpId: PhysicalOpIdentity, newExecInitInfo: OpExecInitInfo)` — no more manual proto-Any serialization.
- `WorkflowReconfigureRequest(reconfiguration, reconfigurationId)` is the existing controller RPC.
- After dispatching, `ExecutionReconfigurationStore` is reset with the new `currentReconfigId` so the diff handler at the top of the file can correctly emit `ModifyLogicCompletedEvent`s as workers complete.
- `StateTransferFunc` is no longer threaded through; the new `UpdateExecutorRequest` schema does not carry it. If a future use case needs stateful transfer between executor instances during reconfig, we can extend the proto then. The pending-reconfigurations list still stores `Option[StateTransferFunc]` (kept for source compatibility), it is simply discarded at dispatch time.
### Any related issues, documentation, discussions?
Follow-up to #4220. See discussion #4016.
### How was this PR tested?
- Engine-side reconfiguration is already covered by `amber/src/test/scala/org/apache/texera/amber/engine/e2e/ReconfigurationSpec.scala` (added in #4220), exercising both Java and Python operators and the source-operator rejection path. Those tests run through `controllerInterface.reconfigureWorkflow` directly.
- This PR's change is purely the web-service-layer wiring that calls into that already-tested controller RPC. The full UI flow (pause → modify-operator-property → resume → see completion event) cannot be unit-tested from this layer in isolation without spinning up the full WebSocket + AmberClient stack.
- `sbt scalafmtCheckAll` clean. Local compile of the touched module requires jOOQ codegen against a Postgres `texera_db`; relying on CI for the full compile.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-7)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #4531:
URL: https://github.com/apache/texera/pull/4531#issuecomment-4322744668
Added a smoke test: `ExecutionReconfigurationServiceSpec` verifying the empty-short-circuit path doesn't dereference `client` or `workflow` and leaves the reconfiguration store unchanged.
I deliberately scoped the unit test to that one invariant: the dispatch path that calls `controllerInterface.reconfigureWorkflow` is hard to mock at this layer (constructing `AmberClient` requires an `ActorSystem` plus an actor handshake at construction time). The engine-side `ReconfigurationSpec` already covers the controller RPC end-to-end with both Java and Python operators.
Open to refactoring the constructor to take `ControllerServiceFs2Grpc[Future, Unit]` instead of the whole `AmberClient` if reviewers prefer — that would let us stub the interface cleanly. Happy to do it in a follow-up if it's preferred separate from this fix.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang merged PR #4531:
URL: https://github.com/apache/texera/pull/4531
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #4531:
URL: https://github.com/apache/texera/pull/4531#issuecomment-4332758701
The `ReconfigurationSpec` fails frequently. I'm not sure if it's due to this PR or #4220.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #4531:
URL: https://github.com/apache/texera/pull/4531#issuecomment-4323405373
> looks good can we leave a TODO comment to say that the state transfer function for java/scala operators are not implemented
yup already added comment
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): wire ExecutionReconfigurationService back to the engine [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #4531:
URL: https://github.com/apache/texera/pull/4531#issuecomment-4332769440
yeah I am looking into it. it is indeed flaky
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org