aglinxinyuan opened a new pull request, #7119:
URL: https://github.com/apache/texera/pull/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 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 every service's classpath 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 deliberately
dependency-free for the same reason — everything depends on it.
**Two retry sites deliberately left alone**, because converting them changes
behavior rather than just structure:
| 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`) 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.
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/test" "FileService/testOnly
org.apache.texera.service.FileServiceSpec" "WorkflowCore/testOnly
org.apache.texera.amber.core.storage.util.LakeFSStorageClientSpec"
```
```
[info] Tests: succeeded 10, 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)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
