1991santhu commented on PR #11654:
URL: https://github.com/apache/seatunnel/pull/11654#issuecomment-5192840167
Thanks, this is a genuinely good catch and all five are addressed.
**Issue 1, the dead zone.** You're right, and it's the worst case to get
wrong. With `rate_limit_backoff_ms >= MAX_BACKOFF_MILLIS`, `waitMillis` is
pinned to the cap from the first retry, `floor` becomes `>= waitMillis`, and
the method returned the cap with no jitter. So the operators running the most
conservative backoff, the ones most likely to be sitting in sustained rate
limiting, got the old lockstep behaviour back.
Took your Option A. The floor is half the wait, raised to
`rate_limit_backoff_ms` only when that is below the wait:
```java
long floor = waitMillis / 2;
if (rateLimitBackoffMs > floor && rateLimitBackoffMs < waitMillis) {
floor = rateLimitBackoffMs;
}
```
Once the cap has pushed the wait under the configured backoff, that
guarantee cannot be honoured anyway, so flooring there only removed the jitter.
There's a regression test for it. I checked it fails on the old code first,
with `observed only [300000]`.
**Issue 2, the description.** Also right, and I should have caught that when
the approach changed. The description still described the equal-jitter version
that got replaced. Rewritten to state the shipped behaviour and the actual
worst case: with the defaults, 210000ms across three retries becomes 420000ms
until the cap absorbs it. I've called that out explicitly for anyone tuning
against checkpoint timeouts.
**Issue 4, sink coverage.** Mirrored all seven jitter tests into
`AirtableSinkWriterTest`, which had none despite carrying a byte-for-byte copy
of the method. Agreed that extracting the duplicate into one shared helper is
the better long-term answer; I've left that out of this PR to keep the diff
focused, happy to follow up.
**Issues 3 and 5.** Blank line removed from the `java.*` import group and
`spotless:apply` run. Comment corrected, the range at retry 3 with a 1000ms
base is `[4000, 8000]`, not 2000ms wide.
18 tests pass across both classes on JDK 11.
On CI: Actions are not enabled on my fork, which is why there's no Build
signal. I'll get that sorted so there's a real run against this branch.
--
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]
