1991santhu opened a new pull request, #1714: URL: https://github.com/apache/iceberg-go/pull/1714
Follow-up to #1640, addressing the review @laskoviymishka left alongside the approval. Docs and tests only — no behaviour change. ## The open question: is the downward-spread branch worth its complexity? Keeping it, and the reason is now a test rather than an argument. The concern was fair: under the defaults (`100ms` / `1m` / `4 retries`) the sequence tops out at 800ms and never saturates, so that branch is unreachable in a normal deployment, and it is the most intricate part of the helper. The previously pinned case (`100ms`/`1s`) also could not distinguish it from a flat floor, because a clean power-of-two ratio puts `d/2` and the last uncapped interval close together. A non-power-of-two ratio separates them. At `minWait=300ms`, `maxWait=1s` the sequence runs 300ms, 600ms, then saturates: - last uncapped interval = **600ms** - `d/2` = **500ms** So `max(minWait, d/2)` would let the first capped attempt draw as low as 500ms, below what the attempt *before* it already guaranteed. That is the inversion the replay exists to prevent. `TestApplyJitterAtCapFloorsCorrectlyForNonPowerOfTwoRatios` pins it; deleting the replay loop makes it fail with draws around 560–590ms against the 600ms floor. The branch is still unreachable on defaults, so that is now stated plainly at the top of it instead of being left for the next reader to derive. ## Doc corrections **The invariant list was wrong.** It claimed the result never falls below `minWait`. That does not hold when `minWait > maxWait` — `calculateBackoff` resolves that configuration to `maxWait` and `applyJitter` returns exactly `maxWait`, which is below the configured minimum. `TestApplyJitterHonoursMinWaitAboveMaxWait` already pinned that behaviour, so the comment contradicted the test. Now qualified: never below `minWait` when `minWait <= maxWait`, otherwise exactly `maxWait`, because no value honours both bounds. **Divergence from Java is now recorded.** `Tasks.exponentialBackoff` jitters by roughly 10% of the current delay; this draws from `[d, 2d]`. The wider window is deliberate, but it means Go and Java clients polling the same metastore spread differently, and that should not be discovered by surprise. ## Test corrections - `TestApplyJitterAtCapStaysWithinBoundsAndVaries` asserted `>= maxWait/2` (500ms) when the guaranteed floor is 800ms, so a regression letting the spread dip to 600–700ms would have passed. Tightened to 800ms so it agrees with the pinned test. Its message was also upper-bound wording on a `GreaterOrEqual` check, which would have made a failure dump confusing. - `TestAcquireLocksAggregateRetryDelayStaysWithinBound` asserted real elapsed wall-clock time. It slept on every run and the lower bound could flake on a loaded runner. It is now `TestAcquireLocksRunsTheFullRetrySchedule` and counts `CheckLock` calls instead — the additive never-shorter property is already covered by `TestApplyJitterBelowCapNeverShorterThanInput` without touching the clock. ## Not included The two Java-compat gaps — backoff scale 2.0 vs Java's 1.5, and the lock-check config keys not matching Java's names and defaults — are behaviour changes rather than documentation, so they are filed separately rather than folded in here. ## Testing `gofmt` and `go vet` clean. `go test ./catalog/hive/ -count=3` passes. The new non-power-of-two test was checked against the change it guards: with the replay loop removed it fails, and it passes with it restored. -- 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]
