You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/08/12 18:26:01 UTC
(iceberg-go) branch main updated: fix(hive): use 1.5 lock-check backoff scale to match Java (#1756)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 09810e289 fix(hive): use 1.5 lock-check backoff scale to match Java (#1756)
09810e289 is described below
commit 09810e2899c71d2089109cfb758ca6615405a6c9
Author: Digvijay <di...@yahoo.com>
AuthorDate: Wed Aug 12 13:25:54 2026 -0500
fix(hive): use 1.5 lock-check backoff scale to match Java (#1756)
Align calculateBackoff with MetastoreLock's checkLock retry schedule
(scaleFactor 1.5) and keep the jitter floor replay on the same sequence.
---
catalog/hive/lock.go | 51 +++++++++++++++++----------
catalog/hive/lock_test.go | 87 +++++++++++++++++++++++++----------------------
catalog/hive/options.go | 4 +++
3 files changed, 84 insertions(+), 58 deletions(-)
diff --git a/catalog/hive/lock.go b/catalog/hive/lock.go
index 16924d156..6dc65dd3b 100644
--- a/catalog/hive/lock.go
+++ b/catalog/hive/lock.go
@@ -158,11 +158,21 @@ func calculateBackoff(attempt int, minWait, maxWait time.Duration) time.Duration
if attempt <= 0 || minWait <= 0 {
return minWait
}
- if attempt > 62 || minWait > maxWait>>attempt {
- return maxWait
+
+ // Match Java's lock-check schedule: minWait * 1.5^attempt, capped at maxWait.
+ // (MetastoreLock uses scaleFactor 1.5 for checkLock retries; the lock-create
+ // path still uses 2.0, and is not mirrored here.)
+ wait := float64(minWait)
+ for i := 0; i < attempt; i++ {
+ next := wait * lockCheckBackoffScale
+ // next <= wait catches +Inf / non-advancing overflow on extreme inputs.
+ if next >= float64(maxWait) || next <= wait {
+ return maxWait
+ }
+ wait = next
}
- return minWait << attempt
+ return time.Duration(wait)
}
// applyJitter spreads a backoff interval so clients contending for the same lock
@@ -213,30 +223,35 @@ func applyJitter(d, minWait, maxWait time.Duration) time.Duration {
// Everything below this point is unreachable under the default configuration.
// lock-check-min-wait-time=100ms, lock-check-max-wait-time=1m and
- // lock-check-retries=4 top the sequence out at 800ms, so it never saturates and
+ // lock-check-retries=4 top the sequence out at 337.5ms, so it never saturates and
// the branch above always wins. Getting here needs a lowered maximum or a raised
// retry count. Stated plainly because this is the most intricate part of the
// helper and the least exercised in practice.
//
- // 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.
+ // Replay the 1.5× backoff sequence and keep the largest interval that still
+ // fitted under the cap. The guard on scheduled keeps a non-positive or
+ // non-advancing minWait from spinning here.
//
- // The replay is what a flat max(minWait, d/2) floor cannot do, and the gap is
- // not hypothetical: at minWait=300ms, maxWait=1s the sequence runs 300ms, 600ms,
- // then saturates. The last uncapped interval is 600ms, but d/2 is 500ms, so a
- // flat floor would let the first capped attempt wait less than the one before it.
- // The divergence appears whenever the ratio is not a clean power of two.
+ // The replay is what a flat max(minWait, d/scale) floor cannot do, and the gap
+ // is not hypothetical: at minWait=300ms, maxWait=1s the sequence runs 300ms,
+ // 450ms, 675ms, then saturates. The last uncapped interval is 675ms, but
+ // d/1.5 is ~666ms, so a flat floor would let the first capped attempt wait less
+ // than the one before it.
//
// minWait is applied before the replay rather than relying on it. When minWait
// is itself >= maxWait the loop cannot run at all, and options.go accepts that
- // configuration, so leaving the floor at d/2 there would allow a wait of a third
- // of the configured minimum.
- floor := max(minWait, d/2)
- for scheduled := minWait; scheduled > 0 && scheduled < maxWait; scheduled <<= 1 {
- if scheduled > floor {
- floor = scheduled
+ // configuration, so leaving the floor at d/scale there would allow a wait below
+ // the configured minimum.
+ floor := max(minWait, time.Duration(float64(d)/lockCheckBackoffScale))
+ for scheduled := float64(minWait); scheduled > 0 && scheduled < float64(maxWait); {
+ if s := time.Duration(scheduled); s > floor {
+ floor = s
+ }
+ next := scheduled * lockCheckBackoffScale
+ if next <= scheduled {
+ break
}
+ scheduled = next
}
if floor >= d {
return d
diff --git a/catalog/hive/lock_test.go b/catalog/hive/lock_test.go
index 32a100722..e87b1fa8c 100644
--- a/catalog/hive/lock_test.go
+++ b/catalog/hive/lock_test.go
@@ -360,14 +360,15 @@ func TestCalculateBackoff(t *testing.T) {
attempt int
expected time.Duration
}{
- {0, 100 * time.Millisecond}, // 100ms * 2^0 = 100ms
- {1, 200 * time.Millisecond}, // 100ms * 2^1 = 200ms
- {2, 400 * time.Millisecond}, // 100ms * 2^2 = 400ms
- {3, 800 * time.Millisecond}, // 100ms * 2^3 = 800ms
- {4, 1 * time.Second}, // 100ms * 2^4 = 1.6s, capped at 1s
- {10, 1 * time.Second}, // Capped at maxWait
- {63, 1 * time.Second}, // Capped without overflowing time.Duration
- {1000, 1 * time.Second}, // Large retry counts remain capped
+ {0, 100 * time.Millisecond}, // 100ms * 1.5^0
+ {1, 150 * time.Millisecond}, // 100ms * 1.5^1
+ {2, 225 * time.Millisecond}, // 100ms * 1.5^2
+ {3, 337500 * time.Microsecond}, // 100ms * 1.5^3
+ {4, 506250 * time.Microsecond}, // 100ms * 1.5^4
+ {5, 759375 * time.Microsecond}, // 100ms * 1.5^5
+ {6, 1 * time.Second}, // 100ms * 1.5^6 ≈ 1.14s, capped at 1s
+ {10, 1 * time.Second}, // Capped at maxWait
+ {1000, 1 * time.Second}, // Large retry counts remain capped
}
for _, tt := range tests {
@@ -378,8 +379,8 @@ func TestCalculateBackoff(t *testing.T) {
}
maxDuration := time.Duration(1<<63 - 1)
- assert.Equal(t, time.Duration(1)<<62, calculateBackoff(62, time.Nanosecond, maxDuration))
- assert.Equal(t, maxDuration, calculateBackoff(63, time.Nanosecond, maxDuration))
+ assert.Equal(t, maxDuration, calculateBackoff(10000, time.Nanosecond, maxDuration),
+ "extreme attempt counts must cap without overflowing")
}
func TestLockConfigurationParsing(t *testing.T) {
@@ -431,13 +432,14 @@ func TestApplyJitterAtCapStaysWithinBoundsAndVaries(t *testing.T) {
minWait := 100 * time.Millisecond
maxWait := time.Second
- // 800ms is the last interval the sequence produced before saturating, which is
- // the floor the code actually guarantees. Asserting maxWait/2 here would pass a
- // regression that let the spread dip to 600ms.
+ // 759.375ms is the last interval the 1.5× sequence produced before saturating,
+ // which is the floor the code actually guarantees. Asserting maxWait/1.5 here
+ // would pass a regression that let the spread dip below that interval.
+ lastUncapped := 759375 * time.Microsecond
seen := make(map[time.Duration]struct{})
for i := 0; i < 500; i++ {
got := applyJitter(maxWait, minWait, maxWait)
- assert.GreaterOrEqual(t, got, 800*time.Millisecond,
+ assert.GreaterOrEqual(t, got, lastUncapped,
"the spread at the cap must be at least the last uncapped interval")
assert.LessOrEqual(t, got, maxWait,
"the wait must never exceed the configured maximum")
@@ -461,26 +463,28 @@ func TestApplyJitterAtCapNeverPollsSoonerThanMinWait(t *testing.T) {
}
// The guaranteed minimum must not fall as the sequence saturates. The attempt
-// before the cap waits for its own full interval, so flooring the spread at half
-// the cap would let the first saturated attempt poll sooner than the one before
-// it, which inverts the property that the wait between checks only ever grows.
+// before the cap waits for its own full interval, so flooring the spread at
+// maxWait/1.5 would let the first saturated attempt poll sooner than the one
+// before it, which inverts the property that the wait between checks only ever
+// grows.
//
// Reaching the cap takes a lowered lock-check-max-wait-time or a raised retry
-// count; the defaults (100ms, 1 minute, 4 retries) top out at 800ms and never
+// count; the defaults (100ms, 1 minute, 4 retries) top out at 337.5ms and never
// saturate. The values below are the smallest that put the boundary in range.
func TestApplyJitterMinimumIsMonotonicAcrossTheCap(t *testing.T) {
minWait := 100 * time.Millisecond
maxWait := time.Second
- // The sequence runs 100ms, 200ms, 400ms, 800ms, then saturates at 1s.
- // Attempt 3 draws from [800ms, 1s]; attempt 4 is the first capped one.
+ // The sequence runs 100ms, 150ms, 225ms, 337.5ms, 506.25ms, 759.375ms, then
+ // saturates at 1s. Attempt 5 draws from [759.375ms, 1s]; attempt 6 is the
+ // first capped one.
//
// The bound compared against is exact, not sampled. Below the cap the jitter is
// added on top, so the interval itself is the floor and no estimate is needed.
// Sampling both sides instead would compare two noisy minima that sit a hair
// above the same true floor, and which of them lands lower is chance.
var floor time.Duration
- for attempt := 0; attempt < 8; attempt++ {
+ for attempt := 0; attempt < 10; attempt++ {
d := calculateBackoff(attempt, minWait, maxWait)
for i := 0; i < 2000; i++ {
@@ -499,36 +503,41 @@ func TestApplyJitterMinimumIsMonotonicAcrossTheCap(t *testing.T) {
func TestApplyJitterAtCapFloorsAtTheLastUncappedInterval(t *testing.T) {
minWait := 100 * time.Millisecond
maxWait := time.Second
+ lastUncapped := 759375 * time.Microsecond
- assert.Equal(t, 800*time.Millisecond, calculateBackoff(3, minWait, maxWait),
+ assert.Equal(t, lastUncapped, calculateBackoff(5, minWait, maxWait),
"the last interval before the cap")
- assert.Equal(t, maxWait, calculateBackoff(4, minWait, maxWait),
+ assert.Equal(t, maxWait, calculateBackoff(6, minWait, maxWait),
"the first interval at the cap")
for i := 0; i < 2000; i++ {
- assert.GreaterOrEqual(t, applyJitter(maxWait, minWait, maxWait), 800*time.Millisecond,
+ assert.GreaterOrEqual(t, applyJitter(maxWait, minWait, maxWait), lastUncapped,
"the spread at the cap must not reach below the last uncapped interval")
}
}
-// The case above uses a clean power-of-two ratio, where d/2 and the last uncapped
-// interval happen to be close. This one separates them: at 300ms/1s the sequence
-// runs 300ms, 600ms, then saturates, so the floor must be 600ms while d/2 is only
-// 500ms. It is the case that distinguishes the replay from a flat max(minWait, d/2)
-// floor, and the only place the arithmetic could quietly go wrong.
-func TestApplyJitterAtCapFloorsCorrectlyForNonPowerOfTwoRatios(t *testing.T) {
+// A flat max(minWait, d/1.5) floor is close but not exact. At 300ms/1s the
+// sequence runs 300ms, 450ms, 675ms, then saturates, so the floor must be 675ms
+// while d/1.5 is only ~666ms. It is the case that distinguishes the replay from a
+// flat scale-factor floor, and the place the arithmetic could quietly go wrong.
+func TestApplyJitterAtCapFloorsCorrectlyVersusFlatScaleFloor(t *testing.T) {
minWait := 300 * time.Millisecond
maxWait := time.Second
+ lastUncapped := 675 * time.Millisecond
- require.Equal(t, 600*time.Millisecond, calculateBackoff(1, minWait, maxWait),
+ require.Equal(t, lastUncapped, calculateBackoff(2, minWait, maxWait),
"the last interval before the cap")
- require.Equal(t, maxWait, calculateBackoff(2, minWait, maxWait),
+ require.Equal(t, maxWait, calculateBackoff(3, minWait, maxWait),
"the first interval at the cap")
+ flatScaleFloor := time.Duration(float64(maxWait) / lockCheckBackoffScale)
+ require.Less(t, flatScaleFloor, lastUncapped,
+ "precondition: the flat floor must sit below the last uncapped interval")
+
for i := 0; i < 2000; i++ {
got := applyJitter(maxWait, minWait, maxWait)
- assert.GreaterOrEqual(t, got, 600*time.Millisecond,
- "a flat d/2 floor would allow 500ms here, below what the previous attempt guaranteed")
+ assert.GreaterOrEqual(t, got, lastUncapped,
+ "a flat d/1.5 floor would allow ~666ms here, below what the previous attempt guaranteed")
assert.LessOrEqual(t, got, maxWait,
"the wait must never exceed the configured maximum")
}
@@ -553,12 +562,10 @@ func TestApplyJitterHonoursMinWaitAboveMaxWait(t *testing.T) {
}
}
-// Exercises the wiring at the acquireLocks call site rather than the helper alone,
-// and pins the aggregate delay. The schedule is 10ms, 20ms, 40ms, 80ms, and each
-// draw adds up to one further interval, so the total falls in [150ms, 300ms]. Only
-// the lower bound is asserted tightly; the ceiling is loose because a busy CI box
-// can add arbitrary scheduling delay on top, and a flaky timing test is worse than
-// no timing test.
+// Exercises the wiring at the acquireLocks call site rather than the helper alone.
+// With minWait=10ms and 4 retries the 1.5× schedule is 10ms, 15ms, 22.5ms, 33.75ms
+// before jitter. Call counts are asserted rather than wall-clock delay, because a
+// busy CI box can add arbitrary scheduling latency on top.
func TestAcquireLocksRunsTheFullRetrySchedule(t *testing.T) {
mockClient := new(mockHiveClient)
ctx := context.Background()
diff --git a/catalog/hive/options.go b/catalog/hive/options.go
index 39809354e..bf1aaf256 100644
--- a/catalog/hive/options.go
+++ b/catalog/hive/options.go
@@ -55,6 +55,10 @@ const (
DefaultLockCheckMinWaitTime = 100 * time.Millisecond // 100ms
DefaultLockCheckMaxWaitTime = 60 * time.Second // 1 minute
DefaultLockCheckRetries = 4
+
+ // lockCheckBackoffScale matches the Java MetastoreLock lock-check path, which
+ // passes 1.5 to Tasks.exponentialBackoff (not the Tasks default of 2.0).
+ lockCheckBackoffScale = 1.5
)
type HiveOptions struct {