laskoviymishka commented on code in PR #1454:
URL: https://github.com/apache/iceberg-go/pull/1454#discussion_r3570292655
##########
table/table.go:
##########
@@ -729,34 +781,44 @@ func readRetryConfig(props iceberg.Properties)
retryConfig {
// concurrent Go writers. Backoff is client-local, so this does not
// affect cross-client interop.
//
-// Inputs are trusted: readRetryConfig is responsible for normalizing
-// user-supplied properties (negatives, zero, min > max).
-func backoffDuration(attempt, minMs, maxMs uint) time.Duration {
+// Inputs from readRetryConfig are validated. The defensive bounds below also
+// keep direct callers from reaching an invalid random bound or duration.
+func backoffDuration(attempt uint, minMs, maxMs uint64) time.Duration {
if minMs == 0 {
minMs = CommitMinRetryWaitMsDefault
}
if maxMs == 0 {
maxMs = CommitMaxRetryWaitMsDefault
}
+ if minMs > maxRetryDurationMs {
+ minMs = maxRetryDurationMs
+ }
+ if maxMs > maxRetryDurationMs {
+ maxMs = maxRetryDurationMs
+ }
if minMs > maxMs {
minMs = maxMs
}
- // Cap the shift count so the signed int64 below does not overflow
- // past its operand width; overflow would just be clamped to maxMs
- // anyway, so keep the math obvious instead.
+ // Cap the shift count so the exponential calculation stays within the
+ // bounded duration range.
if attempt > 62 {
attempt = 62
}
- ceiling := int64(minMs) << attempt
- if ceiling <= 0 || ceiling > int64(maxMs) {
- ceiling = int64(maxMs)
+ var ceiling uint64
+ if minMs > maxRetryDurationMs>>attempt {
+ ceiling = maxMs
+ } else {
+ ceiling = minMs << attempt
+ if ceiling > maxMs {
+ ceiling = maxMs
+ }
}
// Jitter in [minMs, ceiling]: keeps a non-zero floor so concurrent
// writers don't all sample 0 and retry in lockstep.
//nolint:gosec // non-security randomness, jitter for retry spread
- wait := int64(minMs) + rand.Int64N(ceiling-int64(minMs)+1)
+ wait := int64(minMs) + rand.Int64N(int64(ceiling-minMs+1))
Review Comment:
The `//nolint:gosec` is justified in the comment for `rand.Int64N`
(non-security randomness), but it's line-level, so it's also silently
swallowing the G115 warnings on the two uint64→int64 conversions here
(`int64(minMs)` and `int64(ceiling-minMs+1)`). Those are safe — both operands
are bounded by `maxRetryDurationMs <= MaxInt64` — but the comment doesn't say
so, so a reader can't tell the suppression is intentional for them.
I'd expand the justification to mention the int64 conversions are safe
because the operands are `<= maxRetryDurationMs`. Small thing.
##########
table/table.go:
##########
@@ -705,20 +709,68 @@ func rebuildSnapshotUpdates(ctx context.Context, updates
[]Update, freshMeta Met
return result, orphanedPaths, nil
}
+const maxRetryDurationMs = uint64(math.MaxInt64 / int64(time.Millisecond))
+
type retryConfig struct {
numRetries uint
- minWaitMs uint
- maxWaitMs uint
- totalTimeoutMs uint
+ minWaitMs uint64
+ maxWaitMs uint64
+ totalTimeoutMs uint64
}
-func readRetryConfig(props iceberg.Properties) retryConfig {
- return retryConfig{
- numRetries: iceberg.PropUInt(props, CommitNumRetriesKey,
CommitNumRetriesDefault),
- minWaitMs: iceberg.PropUInt(props,
CommitMinRetryWaitMsKey, CommitMinRetryWaitMsDefault),
- maxWaitMs: iceberg.PropUInt(props,
CommitMaxRetryWaitMsKey, CommitMaxRetryWaitMsDefault),
- totalTimeoutMs: iceberg.PropUInt(props,
CommitTotalRetryTimeoutMsKey, CommitTotalRetryTimeoutMsDefault),
+func readRetryConfig(props iceberg.Properties) (retryConfig, error) {
+ numRetries := iceberg.PropUInt64(props, CommitNumRetriesKey,
CommitNumRetriesDefault)
+ if numRetries >= uint64(^uint(0)) {
+ return retryConfig{}, fmt.Errorf(
+ "invalid retry property %q=%d: retry count overflows
the attempt count",
+ CommitNumRetriesKey, numRetries)
+ }
+
+ cfg := retryConfig{
+ numRetries: uint(numRetries),
+ minWaitMs: iceberg.PropUInt64(props,
CommitMinRetryWaitMsKey, CommitMinRetryWaitMsDefault),
+ maxWaitMs: iceberg.PropUInt64(props,
CommitMaxRetryWaitMsKey, CommitMaxRetryWaitMsDefault),
+ totalTimeoutMs: iceberg.PropUInt64(props,
CommitTotalRetryTimeoutMsKey, CommitTotalRetryTimeoutMsDefault),
}
+
+ if cfg.minWaitMs == 0 {
+ cfg.minWaitMs = CommitMinRetryWaitMsDefault
+ }
+ if cfg.maxWaitMs == 0 {
+ cfg.maxWaitMs = CommitMaxRetryWaitMsDefault
+ }
+
+ for _, property := range []struct {
+ key string
+ value uint64
+ }{
+ {CommitMinRetryWaitMsKey, cfg.minWaitMs},
+ {CommitMaxRetryWaitMsKey, cfg.maxWaitMs},
+ {CommitTotalRetryTimeoutMsKey, cfg.totalTimeoutMs},
+ } {
+ key, value := property.key, property.value
+ if value > maxRetryDurationMs {
+ return retryConfig{}, fmt.Errorf(
+ "invalid retry property %q=%d: exceeds maximum
duration of %d milliseconds",
+ key, value, maxRetryDurationMs)
+ }
+ }
+
+ if cfg.minWaitMs > cfg.maxWaitMs {
+ return retryConfig{}, fmt.Errorf(
+ "invalid retry properties %q=%d and %q=%d: minimum wait
exceeds maximum wait",
+ CommitMinRetryWaitMsKey, cfg.minWaitMs,
+ CommitMaxRetryWaitMsKey, cfg.maxWaitMs)
+ }
+
+ jitterSpan := cfg.maxWaitMs - cfg.minWaitMs + 1
+ if jitterSpan == 0 || jitterSpan > uint64(math.MaxInt64) {
Review Comment:
I don't think this can ever fire. By the time we get here both `minWaitMs`
and `maxWaitMs` are validated `<= maxRetryDurationMs` (~9.2e12) and `min <=
max` is enforced just above, so `jitterSpan` is at least 1 and at most ~9.2e12
— never 0, never past MaxInt64 (~9.2e18).
The real int64 boundary is the `int64(ceiling-minMs+1)` conversion in
`backoffDuration`, not here, so this check also points a reader at the wrong
place. I'd drop the block; if we want a marker, a one-line comment noting the
span is always int64-representable given the `maxRetryDurationMs` bound is more
honest than a guard that can't trigger. wdyt?
##########
table/table.go:
##########
@@ -705,20 +709,68 @@ func rebuildSnapshotUpdates(ctx context.Context, updates
[]Update, freshMeta Met
return result, orphanedPaths, nil
}
+const maxRetryDurationMs = uint64(math.MaxInt64 / int64(time.Millisecond))
+
type retryConfig struct {
numRetries uint
- minWaitMs uint
- maxWaitMs uint
- totalTimeoutMs uint
+ minWaitMs uint64
+ maxWaitMs uint64
+ totalTimeoutMs uint64
}
-func readRetryConfig(props iceberg.Properties) retryConfig {
- return retryConfig{
- numRetries: iceberg.PropUInt(props, CommitNumRetriesKey,
CommitNumRetriesDefault),
- minWaitMs: iceberg.PropUInt(props,
CommitMinRetryWaitMsKey, CommitMinRetryWaitMsDefault),
- maxWaitMs: iceberg.PropUInt(props,
CommitMaxRetryWaitMsKey, CommitMaxRetryWaitMsDefault),
- totalTimeoutMs: iceberg.PropUInt(props,
CommitTotalRetryTimeoutMsKey, CommitTotalRetryTimeoutMsDefault),
+func readRetryConfig(props iceberg.Properties) (retryConfig, error) {
+ numRetries := iceberg.PropUInt64(props, CommitNumRetriesKey,
CommitNumRetriesDefault)
+ if numRetries >= uint64(^uint(0)) {
Review Comment:
This guard doesn't buy us much and has a surprising edge. On a 64-bit build
`^uint(0)` is MaxUint64, so `>=` only rejects that exact value — MaxUint64-1
sails through, then `numRetries+1` (the totalAttempts count in the loop) wraps
to 0 and we silently run zero attempts, so the commit never executes and no
error surfaces.
I'd cap to a sane practical bound instead of the type width — reject
`numRetries > math.MaxUint32`, or at minimum guard `numRetries+1 == 0`. A retry
count anywhere near 2^32 is a misconfiguration regardless of platform width.
wdyt?
##########
table/commit_retry_test.go:
##########
@@ -298,16 +299,87 @@ func TestBackoffDuration_HandlesZeroInputs(t *testing.T) {
func TestReadRetryConfig_ClampsNegativeProperties(t *testing.T) {
// Negative values in properties should be replaced with defaults.
- cfg := readRetryConfig(iceberg.Properties{
+ cfg, err := readRetryConfig(iceberg.Properties{
CommitNumRetriesKey: "-1",
CommitMinRetryWaitMsKey: "-100",
CommitMaxRetryWaitMsKey: "-1000",
CommitTotalRetryTimeoutMsKey: "-5",
})
+ require.NoError(t, err)
assert.Equal(t, uint(CommitNumRetriesDefault), cfg.numRetries)
- assert.Equal(t, uint(CommitMinRetryWaitMsDefault), cfg.minWaitMs)
- assert.Equal(t, uint(CommitMaxRetryWaitMsDefault), cfg.maxWaitMs)
- assert.Equal(t, uint(CommitTotalRetryTimeoutMsDefault),
cfg.totalTimeoutMs)
+ assert.Equal(t, uint64(CommitMinRetryWaitMsDefault), cfg.minWaitMs)
+ assert.Equal(t, uint64(CommitMaxRetryWaitMsDefault), cfg.maxWaitMs)
+ assert.Equal(t, uint64(CommitTotalRetryTimeoutMsDefault),
cfg.totalTimeoutMs)
+}
+
+func TestReadRetryConfigRejectsUnsafeProperties(t *testing.T) {
+ maxUint := strconv.FormatUint(math.MaxUint64, 10)
+ maxIntPlusOne := strconv.FormatUint(uint64(math.MaxInt64)+1, 10)
+
+ tests := []struct {
+ name string
+ key string
+ value string
+ }{
+ {name: "minimum wait max uint64", key: CommitMinRetryWaitMsKey,
value: maxUint},
Review Comment:
These use MaxUint64 and MaxInt64+1, but the actual guard is `value >
maxRetryDurationMs`, and `maxRetryDurationMs` is `MaxInt64 / 1e6` (~9.2e12) —
about six orders of magnitude below MaxInt64+1. So nothing here pins the
accept/reject boundary: if the guard were accidentally written as `>
math.MaxInt64` (dropping the divide), every one of these would still pass.
`TestReadRetryConfigAcceptsLargestSafeDuration` nails the accept side at
exactly `maxRetryDurationMs`; I'd add the mirror on the reject side — a case at
`maxRetryDurationMs+1` — so the boundary is actually tested.
##########
types.go:
##########
@@ -93,11 +93,11 @@ func (p Properties) GetInt64(key string, defVal int64)
int64 {
return defVal
}
-// PropUInt reads an unsigned-integer property by key. A missing key,
-// an unparseable value, or a negative value returns defVal — PropUInt
+// PropUInt64 reads an unsigned-integer property by key. A missing key,
+// an unparseable value, or a negative value returns defVal — PropUInt64
// uses strconv.ParseUint, which rejects negatives rather than silently
// wrapping them to a large positive number.
-func PropUInt(p Properties, key string, defVal uint) uint {
+func PropUInt64(p Properties, key string, defVal uint64) uint64 {
Review Comment:
Design question while this is fresh: the other Properties readers are
methods (`GetBool`, `GetInt`, `GetInt64`), but `PropUInt`/`PropUInt64` are
package-level free functions. Now that we're exporting a second one we're
locking the free-function shape in — can't drop it later without a major bump.
I don't feel strongly, and `PropUInt` already set the free-function
precedent, so there's a real consistency-with-precedent argument. But it's
worth a deliberate call: do we want `(p Properties) GetUInt64(...)` to match
the rest of the helpers, or keep the free functions with a one-line note on
why? wdyt?
--
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]
