dongjoon-hyun commented on PR #806:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/806#issuecomment-5458562112
Thank you for working on this, @jiangzho . The direction looks good to me —
honoring the server-provided `retryAfterSeconds` is the right thing to do. I
have one blocker and one design concern.
**1. `retriesCreateOnTransient5xxHonoringRetryAfterWhenPresent` actually
fails (blocker)**
I ran the PR branch locally and this test fails at the timing assertion:
```
ReconcilerUtilsTest >
retriesCreateOnTransient5xxHonoringRetryAfterWhenPresent() FAILED
org.opentest4j.AssertionFailedError at ReconcilerUtilsTest.java:189
```
The test never reaches the backoff sleep. Since 503 is a transient error,
the catch block in `getOrCreateSecondaryResource` performs the GET fallback
**before** the backoff sleep, and the test stubs the second `get()` to return
the pod — so the method returns immediately from inside the catch block
(elapsed ≈ 0ms < 1000ms). To exercise the intended path (503 + Retry-After →
sleep → retry), the second `get()` should return `null` and the second
`create()` should succeed.
**2. The server-provided delay is not capped**
`getRetryAfterMillis` feeds `Thread.sleep` directly. The existing
exponential backoff is capped by
`API_SECONDARY_RESOURCE_CREATE_MAX_BACKOFF_MILLIS`, but the server-provided
value is trusted without a bound. Since this sleep runs on the reconciler
thread, an API server (or an intermediate proxy) responding with e.g.
`retryAfterSeconds: 3600` would block reconciliation for an hour. I'd suggest
applying the existing cap, e.g. `Math.min(retryAfterMillis, maxBackoffMs)`.
**Minor**: the two new `ReconcilerUtilsTest` tests each sleep a real 1s,
adding ~2s to the unit test suite. Probably unavoidable given
`retryAfterSeconds` has whole-second granularity, but worth noting.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
