Skip to content

source: disable DuckLake's internal transaction-retry loop - #47

Merged
jghoman merged 1 commit into
mainfrom
jakob/viaduck-disable-ducklake-internal-retry
Jul 2, 2026
Merged

source: disable DuckLake's internal transaction-retry loop#47
jghoman merged 1 commit into
mainfrom
jakob/viaduck-disable-ducklake-internal-retry

Conversation

@jghoman

@jghoman jghoman commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Sets `ducklake_max_retry_count = 0` on every viaduck DuckLake connection. DuckLake surfaces commit conflicts immediately; viaduck's own `_write_with_retry` remains the sole authority for retry policy.

Why

DuckLake's transaction commit path (`src/storage/ducklake_transaction.cpp` around line 2669) does its own retry-on-conflict loop internally:

```cpp
sleep_amount = retry_wait_ms * random_multiplier * pow(retry_backoff, i);
```

Defaults: `max_retry_count=10`, `retry_wait_ms=100ms`, `retry_backoff=1.5`. Sum of sleeps ≈ 8.5s per failed commit at defaults, all inside the `tbl.append()` / `tbl.upsert()` call, before viaduck sees anything.

That means:

  • Invisible to viaduck's metrics — the retry counter (`viaduck_dest_write_retries_total`) doesn't tick for these
  • Invisible to viaduck's backoff strategy — DuckLake's raw `sleep_for` doesn't participate in viaduck's jittered + capped exponential
  • Invisible to the shutdown event — a worker deep in DuckLake's sleep ignores viaduck's stop_event and k8s' `terminationGracePeriodSeconds`
  • Non-linear at higher settings — the exponent is uncapped, so bumping `max_retry_count` for higher robustness is a trap: `max_retry_count=30` puts the last attempt at ~4h.

Empirically observed on portola tonight: single team-50689 flushes taking 20+ minutes, ~4 min gaps between pool-eviction reconnects, retry counter climbing slowly relative to wall time. Consistent with each viaduck retry attempt burning several seconds inside DuckLake's internal loop before the "Failed to commit DuckLake transaction" error surfaces.

What this does not fix

The underlying reason viaduck loses OCC conflicts against the backfill: viaduck's writes span partitions across many months (the destination table is partitioned by event `timestamp`, which for CDC-replayed events can be any historical date), and each concurrent backfill commit also spans many partitions. Structural collision, not a backoff-tuning problem. That's separate work — either a partition-split arrangement between viaduck and backfill, or waiting out the backfill.

This PR just stops each viaduck retry attempt from wasting 8.5s in the wrong retry loop.

Test plan

  • `just ci` clean: 489 unit + 74 integration tests
  • `test_connect` asserts the new property is passed through pyducklake's ATTACH-time SET machinery
  • Manual: after deploy, confirm `_write_with_retry` WARN log cadence is now much tighter (attempts fire ~seconds apart, not ~30s apart) and `viaduck_dest_write_retries_total` climbs at the same rate as observed catalog snapshot bumps
  • Manual: confirm no functional regression — viaduck still eventually commits when there IS a gap

DuckLake's commit path (ducklake/src/storage/ducklake_transaction.cpp
around line 2669) does its own retry-on-conflict inside the commit call:

    sleep_amount = retry_wait_ms * random_multiplier * pow(retry_backoff, i)

with defaults `max_retry_count=10, retry_wait_ms=100, retry_backoff=1.5`.
At default settings each viaduck flush attempt sits inside DuckLake
sleeping ~8.5s before the "Failed to commit DuckLake transaction" error
finally surfaces. That sleep happens BEFORE viaduck's own retry loop
observes anything, so it's invisible to `_write_with_retry`'s jittered
backoff, invisible to the retry counter, and — critically — ignored by
the shutdown-event path (the sleep is on a raw std::this_thread::sleep_for).

Bumping max_retry_count for higher robustness makes it worse, not better:
the exponent grows uncapped, so `max_retry_count = 30` puts the last
attempt at ~4h.

Set `ducklake_max_retry_count = 0` at connection open in
`_CONNECTION_DEFAULTS` so DuckLake surfaces conflicts immediately.
viaduck's `_write_with_retry` remains the sole authority for retry policy
— it has jitter, a capped backoff, per-attempt metrics, and honors the
shutdown event.

Test: update `test_connect` to assert the new property is passed through
via pyducklake's ATTACH-time SET machinery.
@jghoman
jghoman merged commit 8160c30 into main Jul 2, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant