nsivabalan opened a new pull request, #20028:
URL: https://github.com/apache/hudi/pull/20028
### Describe the issue this Pull Request addresses
Closes #20027.
`StorageBasedLockProvider` leaves a dangling lock file when the storage
backend returns an HTTP 5xx on the lock-expire write. Observed in production: a
writer acquired the lock, completed everything inside the lock, then hit `503
SERVICE_UNAVAILABLE` on release. The lock file was left un-expired, and ~30s
later a second writer detected the dangling lock and alerted.
This is the same class of bug as #18438 / #18439, which fixed it for HTTP
429 only. 5xx was never covered.
**Root cause.** All three storage lock clients classify HTTP 5xx as
`UNKNOWN_ERROR` — the 5xx branch logs a warning and falls through to the
`UNKNOWN_ERROR` return. `tryExpireCurrentLock` maps that to
`ExpireLockResult.FAILED`, and the retry loop added in #18439 only engages
while the result is `THROTTLED`:
```java
for (int attempt = 1; attempt <= THROTTLE_MAX_RETRIES && expireResult ==
ExpireLockResult.THROTTLED; attempt++) {
```
So a 503 fails the release on the **first** attempt, with no retry. The
cloud SDKs cannot cover for this either: both clients are deliberately built
with retries disabled (`S3` `retryStrategy(r -> r.maxAttempts(1))`, `GCS`
`RetrySettings.setMaxAttempts(1)`), so a single transient blip dangles the lock
every time.
**Why retrying is safe.** The expire write is conditional and pinned to the
*same* precondition token on every attempt (`getLock()` is only cleared on
SUCCESS). If the write never landed, the precondition still matches and the
retry succeeds. If it silently did land, the token no longer matches, yielding
`412` -> `ACQUIRED_BY_OTHERS`, which is already handled as released. Either way
the retry converges — the same argument that made the #18439 throttle retry
safe.
### Summary and Changelog
Users running `StorageBasedLockProvider` on S3, GCS or Azure will no longer
get a dangling lock from a single transient 5xx during lock release.
- Added `LockUpsertResult.TRANSIENT_ERROR`, returned for HTTP 5xx by
`S3StorageLockClient`, `GCSStorageLockClient` and `AzureStorageLockClient`.
`409`, client-side timeouts and other 4xx keep their existing `UNKNOWN_ERROR`
behavior.
- `unlock()`: generalized the existing retry loop from THROTTLED-only to any
retriable outcome via `ExpireLockResult.isRetriable()`, keeping the 3-retry
1s/2s/4s budget and the lock-identity guard against a concurrent `tryLock()`.
- `tryLock()`: split into a `synchronized tryLockInternal()` plus a wrapper
that retries once, with the backoff sleep **outside** the provider monitor so a
waiting acquirer never blocks `unlock()` or the heartbeat. `UNKNOWN_ERROR` is
deliberately **not** retried here — the write may have landed, and an identical
retry could act on a lock we cannot reason about.
- `renewLock()`: same split into `renewLockOnce()` plus a bounded in-cycle
retry, sized to stay well inside one heartbeat interval.
- Added a dedicated `lock.transient.error` metric so an unavailable backend
is distinguishable from a rate-limited one and from a genuinely unknown lock
state, plus a `TRANSIENT_RETRIES_EXHAUSTED` cause on the `FAILED_TO_RELEASE`
exception.
Retry budgets differ by path on purpose: a failed `tryLock()` is safe
(callers retry higher up, and another writer can take the lock), whereas a
failed release dangles the lock until its lease elapses.
No code was copied from elsewhere.
### Impact
Reduces dangling locks for `StorageBasedLockProvider` on S3, GCS and Azure
when storage returns a transient 5xx. No behavior change for 429 (already
handled), 409, other 4xx, or client-side timeouts. No config change and no
public API change; `LockUpsertResult` gains an enum value.
`tryLock()` and `renewLock()` are no longer declared `synchronized` — all
state mutation still happens inside the synchronized single-attempt bodies, and
only the backoff sleep sits outside the monitor. Reentrancy is unchanged and
still covered by existing tests.
### Risk Level
Medium.
Verification:
- `TestStorageBasedLockProvider` **56/56**: 503-on-release retry, retry
exhaustion asserting the 1s/2s/4s schedule and the exhausted-transient cause,
the lock-replaced-during-retry-sleep concurrency guard for the transient path,
acquire-path retry, plus negative tests proving contention and `UNKNOWN_ERROR`
are **not** retried.
- `TestHoodieLockMetrics` **14/14**.
- `TestS3StorageLockClient` **20/20**, `TestGCSStorageLockClient` **20/20**,
`TestAzureStorageLockClient` **31/31** — each with a parameterized
500/502/503/504 case so the whole 5xx range is covered, not just the 503 seen
in production.
- Checkstyle clean on all four touched modules. Sleeps are stubbed, so the
suites run in seconds.
### Documentation Update
None required — no new configs and no user-facing config change. One new
metric (`lock.transient.error`) is emitted alongside the existing lock metrics.
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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]
