Skip to content

chore(amber): remove the unused DeployStrategy family - #7705

Open
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:chore/remove-deploy-strategy
Open

chore(amber): remove the unused DeployStrategy family#7705
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:chore/remove-deploy-strategy

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Deletes the deploystrategy package — the DeployStrategy trait, its three implementations (OneOnEach, RandomDeployment, RoundRobinDeployment) and their shared spec. Pure deletion, no behaviour change: −293 lines.

Nothing has called any of them since #1807 (2023-02-02, "[Compiler Refactor 6] Refactor Amber Workflow to use the new PhysicalPlan implementation"). Searching both by symbol name and by package path, the only files that reference the four types are the package's own files and its spec.

Worker placement is now inline. ExecutorDeployment.createWorkers picks an address straight off AddressInfo:

addressInfo.allAddresses(workerIndex % addressInfo.allAddresses.length)

RoundRobinDeployment's logic absorbed into the caller. Coordinator and RegionExecutionManager reach placement through that path and never touch the trait.

Reviewer note — why this looks alive. The package got two fixes in May 2026, #5028 (reset the iteration cursor in OneOnEach.initialize) and #5029 (unify the empty-array errors), plus the unit-test suite from #4723. All three came from starter-task and coverage issues (#4731, #4732, #4722) that polish the code where it sits; none wires it to a caller. Every related issue and PR is closed and nothing open proposes using the trait — but if a worker-placement change is planned that wants this abstraction back, say so and I will close this.

AddressInfo lives one package up in deploysemantics and is not touched — it is what the live path reads.

Any related issues, documentation, discussions?

Closes #7704

How was this PR tested?

Existing tests only — this PR adds none, since it removes code and the spec that covered it.

Locally, from the repo root with Java 17:

  • sbt "WorkflowExecutionService/Test/compile" — success (main and test sources).

Verification, re-runnable by a reviewer:

git grep -w DeployStrategy OneOnEach RandomDeployment RoundRobinDeployment   # only the deleted files
git grep -l deploystrategy                                                   # only the deleted files

CI note: the build jobs currently fail repo-wide at workflow startup — an org policy blocks the injected carabiner-dev/actions/install/ampel action ("not allowed in apache/texera"). main fails identically, so it is unrelated to this change; same class as #6989 and #7572.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

Copilot AI lite review requested due to automatic review settings August 17, 2026 02:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the unused DeployStrategy family from the amber engine

2 participants