1991santhu commented on PR #1640:
URL: https://github.com/apache/iceberg-go/pull/1640#issuecomment-5183753484
Pushed a correction to my own patch. The first revision fixed the transient
case and left the persistent one untouched.
I bounded the added jitter by the headroom to `maxWait`. But
`calculateBackoff` reaches `maxWait` deliberately — three of its branches
return it outright — and at that point the headroom is zero, so the wait came
back unjittered. Every retry past the point where the sequence tops out
therefore put contending clients straight back into lockstep polling, which is
the behaviour this PR exists to remove. With the defaults (100ms / 60s / 4
retries) the sequence never gets there, but `lock-check-retries` is
configurable and `minWait >= maxWait` saturates on the very first attempt.
The wait is now spread downwards when there is no headroom to add into:
```go
floor := d / 2
if minWait > floor {
floor = minWait
}
if floor >= d {
return d
}
return d - time.Duration(rand.Int64N(int64(d-floor)+1))
```
Two things worth calling out about the direction:
`maxWait` is an upper bound on the polling interval rather than a target, so
drawing below it breaks no contract. `minWait` is the opposite — it is a floor,
and the comment above `applyJitter` promises that a caller configuring a
minimum wait never polls sooner than it. So the downward spread has to be
floored at `minWait`, not at `d/2`. That case is genuinely reachable: with
`lock-check-min-wait-time=40s` and `lock-check-max-wait-time=60s`,
`calculateBackoff` returns 60s straight away, and an unfloored spread would
have polled at 30s — sooner than the configured minimum. There is a test for
exactly that.
Below the ceiling the jitter is still added, so the wait is never cut short
of what `calculateBackoff` produced.
I also made an interval handed in already above `maxWait` return unchanged
rather than fall into the downward branch — that is outside the function's
contract and is better left exactly as given than silently reshaped. The
existing `TestApplyJitterRespectsMaxWait` covers it.
`applyJitter` now takes `minWait` as well, so the call site passes
`opts.LockMinWaitTime`.
`go build ./...` and `go test -count=2 -run
"TestApplyJitter|TestCalculateBackoff|TestLock" ./catalog/hive/` both pass.
--
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]
