zeroshade commented on code in PR #1640:
URL: https://github.com/apache/iceberg-go/pull/1640#discussion_r3732154108
##########
catalog/hive/lock.go:
##########
@@ -160,6 +165,75 @@ func calculateBackoff(attempt int, minWait, maxWait
time.Duration) time.Duration
return minWait << attempt
}
+// applyJitter spreads a backoff interval by adding a random amount on top of
it,
+// bounded so the result never exceeds maxWait.
+//
+// calculateBackoff is a pure function of the attempt number and the configured
+// bounds, so every client waiting on the same table lock computes an identical
+// sequence of delays. Contention is the precondition for entering the retry
loop
+// at all, which means those clients are waiting simultaneously by
construction:
+// they re-check the lock in lockstep, and each round of CheckLock calls
arrives
+// at the metastore as a burst. Spreading the wait decorrelates them.
+//
+// The jitter is added rather than subtracted so that the result is never
shorter
+// than the interval calculateBackoff produced. That keeps the guarantee
implied
+// by the lock-check-min-wait-time property: a caller who configures a minimum
+// wait never polls sooner than it.
+//
+// Once the backoff saturates at maxWait there is no headroom left to add into.
+// calculateBackoff reaches that point deliberately, so leaving it unjittered
+// would put contending clients back in lockstep for every retry after the
+// sequence tops out. The wait is therefore spread downwards instead. Drawing
+// below maxWait breaks no contract, because it is an upper bound on the
polling
+// interval rather than a target.
+//
+// How far down it may draw is the subtle part. Flooring at half the interval
is
+// wrong: the last attempt that did not saturate waited for its own full
interval,
+// which is somewhere in [maxWait/2, maxWait), so a floor of maxWait/2 lets the
+// first saturated attempt wait less than the attempt before it did. That
inverts
+// the one property callers rely on, which is that the wait between lock checks
+// only ever grows. The floor is therefore the last interval the doubling
sequence
+// produced before it hit the cap, recovered by replaying the sequence. It is a
+// bound the schedule has already cleared, so honouring it keeps the guaranteed
+// minimum monotonic across the saturation boundary, and it can never fall
below
+// minWait because minWait is where the sequence starts.
+func applyJitter(d, minWait, maxWait time.Duration) time.Duration {
+ if d <= 0 {
+ return d
+ }
+
+ // A caller that hands in an interval already past the cap is outside
the
+ // contract; leave it exactly as given rather than silently reshaping
it.
+ headroom := maxWait - d
+ if headroom < 0 {
+ return d
+ }
+
+ // Add up to another full interval, without exceeding the configured
maximum.
+ extra := d
+ if headroom < extra {
+ extra = headroom
+ }
+ if extra > 0 {
+ return d + time.Duration(rand.Int64N(int64(extra)+1))
+ }
+
+ // Replay the doubling sequence and keep the largest interval that
still fitted
+ // under the cap. The guard on scheduled keeps a non-positive or
overflowing
+ // minWait from spinning here; in that case the half-interval floor
stands.
+ floor := d / 2
Review Comment:
Non-blocking: consider clamping this initial floor to at least `minWait`, or
rejecting `minWait >= maxWait` while parsing the options. With `minWait=90s`
and `maxWait=60s`, `calculateBackoff` returns 60s, the replay guard is false on
entry, and the floor remains 30s. That permits waits down to one third of the
configured minimum, contrary to the documented promise.
`catalog/hive/options.go:91-105` currently makes this configuration reachable.
A focused 90s/60s regression case would also pin the behavior.
##########
catalog/hive/lock.go:
##########
@@ -160,6 +165,75 @@ func calculateBackoff(attempt int, minWait, maxWait
time.Duration) time.Duration
return minWait << attempt
}
+// applyJitter spreads a backoff interval by adding a random amount on top of
it,
Review Comment:
Non-blocking: consider condensing this comment. The detailed floor
derivation is useful design context, but a roughly 31-line comment is
disproportionate to this small helper; the full rationale could live in the
commit message with a shorter invariant-focused note here.
##########
catalog/hive/lock.go:
##########
@@ -100,7 +101,11 @@ func acquireLocks(ctx context.Context, client HiveClient,
identifiers []tableLoc
// If not acquired immediately, wait and retry
for attempt := 0; attempt < opts.LockRetries; attempt++ {
// Wait before checking again
- waitTime := calculateBackoff(attempt, opts.LockMinWaitTime,
opts.LockMaxWaitTime)
+ waitTime := applyJitter(
Review Comment:
Non-blocking: consider adding one `acquireLocks`-level test that exercises
this wiring and cancellation path, plus an assertion that the aggregate retry
delay stays within the intended bound. The helper tests cover the distribution
thoroughly, but do not execute the new call site.
--
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]
