The GitHub Actions job "Required Checks" on texera.git/main has succeeded. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: 2b030973f3a2ab8356307e36a5cd2a5998b1b99d / Xinyuan Lin <[email protected]> refactor(util): host one exponential-backoff retry (#7119) ### What changes were proposed in this PR? The same doubling-backoff loop was written three times, and no module could share the others' copy: amber's `Utils` sits above `common/workflow-core` and `file-service` in the module graph. New dependency-free **`common/util`** module with `RetryUtil.withBackoff`, and both blocking copies now call it: | Site | Before | After | | --- | --- | --- | | `LakeFSStorageClient.healthCheck` | private 27-line `retryWithBackoff` | `RetryUtil.withBackoff("connect to lake fs server", 5, 200, ...)` | | `FileService.awaitDependency` | private 36-line loop | 7-line delegation keeping its 6/200 defaults + logger | | `Utils.retry` (amber, non-blocking) | unchanged | cross-references the blocking sibling | ```scala RetryUtil.withBackoff(description, maxAttempts, initialDelayMillis, onRetry, sleep)(operation) ``` The util owns everything the copies duplicated: attempt counting, the doubling, what counts as transient (`NonFatal`), interrupt fail-fast with the interrupt status restored, and the message wording. `description` is a verb phrase ("connect to lake fs server"), interpolated into all three messages, so LakeFS's give-up text is byte-identical to before. Retries are reported through an `onRetry` hook carrying a `RetryAttempt` (attempt, budget, delay, cause, and the standard `message`), which callers log with **their own** logger — so a LakeFS retry still logs under LakeFS, not under the util. **Bug fixed on the way in.** LakeFS's `sleep` call sat inside the `catch` block, so an interrupt raised *while waiting* escaped raw with the interrupt flag left cleared — a `catch` cannot catch what its own body throws: ``` Before (LakeFS): op fails -> catch -> sleep interrupted -> InterruptedException escapes, flag cleared After (shared): op fails -> sleep interrupted -> interrupt restored + wrapped, fails fast ``` `FileService`'s copy already plugged that hole with an inner `try`; the shared util keeps the better behavior for both. **Why a new module rather than `common/workflow-core`**, which was proposal 1 in #7095: `workflow-core` is not reachable from `Auth`, `ConfigService`, or `AccessControlService`, so hosting it there would have left future callers in those modules stuck writing their own loop — the exact thing this PR is meant to stop. `common/config` is reachable from everything and already carries `org.apache.texera.amber.util.ConfigParserUtil`, but a retry helper isn't configuration. A dependency-free module any other module may depend on avoids both problems, at the cost of one `build.sbt` entry. **Why the non-blocking variant stays in amber.** `Utils.retry` needs `com.twitter:util-core`, declared only in `amber/build.sbt`. Moving it down would put util-core on the classpath of every service that depends on the new module and force LICENSE-binary resyncs, so the pair is: blocking in `common/util`, non-blocking in `amber`, each documented in terms of the other. The new module is dependency-free for the same reason. **One behavior change, deliberate.** Both replaced loops caught `case e: Exception`; the util uses `NonFatal`, for parity with `Utils.retry`. `NonFatal` is wider: non-fatal `Error`s — `AssertionError`, `java.io.IOError`, `ServiceConfigurationError` — are now retried and wrapped rather than propagating immediately. On the S3/LakeFS startup paths that means such a failure costs the full backoff before surfacing. `RetryUtilSpec` pins it in both directions (a non-fatal `Error` is retried; a fatal throwable is not). **Two retry sites deliberately left alone**, because converting them changes behavior rather than just structure — tracked with the full reasoning in #7124: | Site | Why not now | | --- | --- | | `URLFetchUtil.getInputStreamFromURL` | Retries 5x with **no** delay and returns `Option`. Adopting backoff adds up to ~3 s before a dead URL gives up — arguably a fix (it is an unthrottled retry storm today), but a behavior change in an operator path. | | `PythonProxyClient` Flight connect loop | **Constant** delay, closes the Flight client between attempts, and throws `WorkflowRuntimeException` on give-up. Needs a delay-multiplier knob plus a give-up type decision. | ### Any related issues, documentation, discussions? Closes #7095 Follows #7088, which introduced the non-blocking variant; the consolidation was split out of it to keep that fix narrow. Context: the review discussion on #7103 (the LakeFS health-check backport) asked for one util rather than a fourth copy. ### How was this PR tested? `RetryUtilSpec` (new, in `common/util`, registered in `build.yml`'s hand-maintained `jacoco` module list so it actually runs) is the union of what the two hand-rolled loops were tested for, plus what neither covered: - returns the operation's value on first success without sleeping; retries until success with a doubling progression; honors a custom base delay; succeeds on the final permitted attempt (the `attempt >= maxAttempts` boundary); gives up naming the description and preserving the cause; `maxAttempts = 1` means no retry and no sleep; - the `onRetry` hook fires once per wait with 1-based attempt numbers and never after the last attempt, and its `message` wording is pinned; - interrupt during the operation **and** interrupt during a backoff sleep both restore the interrupt status and fail fast (the second is the LakeFS hole above); - a fatal throwable is not retried and passes through unwrapped, while a non-fatal `Error` is retried (the `NonFatal`-vs-`Exception` widening above). At the call sites: `FileServiceSpec` keeps 8 tests that exercise the delegation, its own 6/200 defaults, and the description in its messages (the 4 that only re-tested util mechanics moved into `RetryUtilSpec`). `LakeFSStorageClientSpec`'s 4 retry tests moved out with the loop; its remaining 5 pass unchanged. amber's `UtilsSpec` and `RegionExecutionManagerSpec` are untouched and still green. ```bash sbt "Util/jacoco" "FileService/testOnly org.apache.texera.service.FileServiceSpec" "WorkflowCore/testOnly org.apache.texera.amber.core.storage.util.LakeFSStorageClientSpec" ``` ``` [info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0 (Util) [info] Tests: succeeded 12, failed 0, canceled 0, ignored 0, pending 0 (FileService) [info] Tests: succeeded 5, failed 0, canceled 0, ignored 0, pending 0 (WorkflowCore / LakeFS) ``` Lint plus test-source compiles for every module the new dependency edge touches, and amber's two retry specs: ```bash sbt "Util/scalafixAll --check" "Util/scalafmtCheckAll" "WorkflowCore/Test/compile" "WorkflowCore/scalafixAll --check" "FileService/Test/compile" "FileService/scalafixAll --check" "WorkflowExecutionService/Test/compile" "WorkflowExecutionService/scalafixAll --check" "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.UtilsSpec org.apache.texera.amber.engine.architecture.scheduling.RegionExecutionManagerSpec" ``` ``` [success] all scalafix / scalafmt checks and Test/compile tasks [info] Tests: succeeded 27, failed 0, canceled 0, ignored 0, pending 0 ``` ### 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/30587201537 With regards, GitHub Actions via GitBox
