The GitHub Actions job "Required Checks" on texera.git/gh-readonly-queue/main/pr-7088-4e152e0d823224a42a59ade8abbd1f2c9075ca11 has succeeded. Run started by GitHub user aglinxinyuan (triggered by aglinxinyuan).
Head commit for run: 37ea97d5b9572499b3a7022dc1e621db1cc6c69d / Xinyuan Lin <[email protected]> chore(amber): bound region termination to a short backoff (#7088) ### What changes were proposed in this PR? **Root cause.** `RegionExecutionManager` retried region termination (`EndWorker`, then `gracefulStop`) up to 150 times at a flat 200 ms delay, so a region whose workers do not terminate held teardown for ~30 s before the failure surfaced. Killing a worker is deterministic -- `EndWorker` plus `gracefulStop` either succeed, or something upstream is broken in a way retrying cannot fix (#6891, #6918). The long poll bought nothing except a quiet ~30 s stall that masked the real bug. ``` Before: EndWorker fails -> 149 retries x 200 ms -> ~30 s of silence -> loud failure After: EndWorker fails -> 3 retries (200/400/800 ms) -> ~1.4 s -> loud failure ``` | | Before | After | | --- | --- | --- | | Attempts | 150 | 4 (1 + 3 retries) | | Delay between attempts | flat 200 ms | 200 -> 400 -> 800 ms | | Worst-case wait per region | ~30 s | ~1.4 s | | Give-up behavior | `IllegalStateException` naming stuck workers | unchanged | **The retry itself lives in a util, not in the manager** (per review): `Utils.retry` keeps its `attempts` + `baseBackoffTimeInMS`-doubling contract but now schedules its waits on a `Timer` instead of calling `Thread.sleep`, so it can be used from an actor or coordinator thread -- a blocking sleep there stalls every other message queued on that thread, which is why this path could not reuse the old version. An `onRetry` hook keeps caller context in the log (the manager still names the region and the attempt); callers that do not care can omit it. ```scala Utils.retry(attempts, baseBackoffTimeInMS, timer, onRetry) { operationReturningAFuture } ``` The previous blocking `retry` had no production callers -- only its own tests -- so it was replaced rather than duplicated. A blocking front door can return as a thin sibling if a caller needs one. `RegionExecutionManager` is now just a configuration of that util (4 attempts from a 200 ms base) plus its own give-up message. Two more copies of this loop live elsewhere -- `LakeFSStorageClient.retryWithBackoff` and `FileService.awaitDependency` -- but they sit in `common/workflow-core` and `file-service`, neither of which can see amber's `Utils` (and `util-core` is declared only in `amber/build.sbt`). Folding those in means hosting the util lower in the module graph, tracked in #7095 rather than smuggled into this fix. Log line, before and after: ``` - Failed to terminate region 1 on attempt 1 of 150. Retrying in 200 ms. + Failed to terminate region 1 on attempt 1 of 4. Retrying in 200 ms. + Failed to terminate region 1 on attempt 2 of 4. Retrying in 400 ms. + Failed to terminate region 1 on attempt 3 of 4. Retrying in 800 ms. + Region 1 could not be terminated after 4 attempts; giving up. Workers still not terminated: ... ``` This also shrinks -- but does not close -- the retry-on-`JavaTimer`-thread race window in #6923, since there are far fewer hops onto that thread. ### Any related issues, documentation, discussions? Closes #7087 Related: #7056 (the 150-attempt line appears in those hang logs), #6891, #6918, #6923. The bounded-retry loop this PR re-tunes was introduced in #5737. ### How was this PR tested? `UtilsSpec` covers `Utils.retry` directly: first-attempt success waits not at all; the backoff doubles (`200 -> 400 -> 800`); waiting stops as soon as an attempt succeeds; `onRetry` sees the 1-based failed-attempt number and the upcoming backoff; a *synchronous* throw from the by-name body is retried like a failed `Future` rather than escaping; a *fatal* error is not retried in either shape (it escapes on attempt 1, spending no backoff); and `attempts <= 1` makes exactly one attempt with no wait. The three cases that covered the removed blocking variant are gone with it. `RegionExecutionManagerSpec` keeps its termination-lifecycle cases and pins what the defaults mean end to end: the manager asks for `200 -> 400 -> 800 ms`, makes 4 `EndWorker` attempts, and a region that terminates on attempt 2 spends only the first wait. Budget boundaries are still pinned (success on the last permitted attempt, give-up at 1 attempt / 2 attempts with every stuck worker named). The backoff assertions are exact rather than timing-tolerant: `RecordingInlineTimer` records each requested delay and runs the task inline, used inside `Time.withCurrentTimeFrozen` so `when - Time.now` is precisely what `Future.sleep` asked for. The tests therefore assert the real schedule without waiting 1.4 s or flaking on slow CI. That needed a `killRetryTimer` seam on the constructor (default `new JavaTimer(true)`, as before). The two directly affected specs, with formatting and scalafix in the same invocation: ```bash sbt "WorkflowExecutionService/scalafmtAll" "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.UtilsSpec org.apache.texera.amber.engine.architecture.scheduling.RegionExecutionManagerSpec" "WorkflowExecutionService/scalafixAll --check" ``` ``` [info] Tests: succeeded 27, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` Whole scheduling package plus `UtilsSpec`: ```bash sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.scheduling.* org.apache.texera.amber.engine.common.UtilsSpec" ``` ``` [info] Suites: completed 13, aborted 1 [info] Tests: succeeded 145, failed 0, canceled 0, ignored 0, pending 0 ``` (`DefaultCostEstimatorSpec` aborts on this machine during suite construction, before any of its tests run, while building a CSV scan descriptor. Pre-existing and unrelated: with this PR's changes reverted it aborts identically under the same command, 0 tests run.) ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) Report URL: https://github.com/apache/texera/actions/runs/30511917350 With regards, GitHub Actions via GitBox
