Replace DuckLake with authoritative Parquet storage - #207
Conversation
Move telemetry ingestion to a durable WAL-backed repository that commits indexed hot segments and open Parquet files. Keep DuckDB as the analytical query and rollup engine, and SQLite as control-plane storage. BREAKING CHANGE: existing DuckLake telemetry data is not migrated. Deployments must start with a clean storage.data_dir.
Fail fast on legacy DuckLake data instead of hiding it. Drain and publish compaction without blocking reads or commits, and surface failed writes immediately.
Keep failed ingest batches backpressured until durable commit. Use day-partitioned leveled compaction so retention remains enforceable without rewriting the retained corpus. Hold Parquet snapshot locks through row iteration and bound hot-segment descriptors.
Persist staged directory entries before publishing the recovery marker. Validate every required signal output and restore retired inputs instead of publishing an incomplete compaction.
Prevent compacted WAL replay, bound writer shutdown and poison-batch retries, tier cold log reads through Parquet, and serialize DuckDB maintenance with rollups and context-aware query locks.
Acquire reader exclusion before the storage publication lock and cap each maintenance pass so rollups cannot stall ingest. Keep recovery namespace swaps reader-atomic and escalate sustained maintenance failures.
Apply maintenance budgets only between completed publications so slow compactions make durable progress. Bound rollup admission and publication waits without canceling admitted cache work or removing ingest from rotation.
| return err | ||
| func (d *Duck) lockRollupParquetRead(ctx context.Context) error { | ||
| waitCtx, cancel := context.WithTimeout(ctx, rollupReaderLease) | ||
| defer cancel() |
There was a problem hiding this comment.
HIGH — unbounded rollup snapshot hold vs. a fixed 60s publisher budget (new in fbc56044).
lockRollupParquetRead now bounds only admission (15s); once the reader is admitted the rollup chunk runs under the process context with no deadline. In the same commit rollupChunkNanos went back to 1h (duck.go:903), and refreshEdgeRollup loops over every edgeStartChunkNanos (30 min) sub-window of the affected start_time range inside one transaction while holding parquetMu.RLock.
Meanwhile PublishParquet (duck.go:1462) still caps the publisher at 2*defaultWriterGrace = 60s.
Concrete failure: any instance restarting with a backlog (or after an outage) processes a 1h ingest chunk covering up to ~115M rows at the certified ~32k rows/s ceiling; the edge rollup's sub-window loop over a backfill whose start_time spans days runs hundreds of self-joins in one transaction. That single reader holds the snapshot for minutes. Every PublishParquet queued during it hits the 60s deadline, so retention and compaction make zero progress for the entire catch-up — the storage-growth spiral, now reported only as degraded + HTTP 200.
This is the fifth relocation of the same serialization point rather than a fix: round 5 canceled the rollup at 15s (rollups never committed); round 6 removed the cancel (maintenance never runs). The reader hold and the publisher budget have to be derived from one another, not chosen independently.
| compactStart := time.Now() | ||
| var cleanupErr, parquetErr, compactErr error | ||
| compacted := 0 | ||
| if recoveryErr == nil { |
There was a problem hiding this comment.
HIGH — recoveryErr gates cleanup + prune + compaction, and nothing clears the marker (carried from 526fd87c/895c238a, still open).
if recoveryErr == nil guards CleanupParquet, PruneParquetPass and CompactParquetPass. RecoverParquet only does work when COMPACTION.json exists, and it fails whenever completeCompaction -> PublishReplacement -> PublishParquet times out.
Combined with the reader-hold issue above this latches permanently: a compaction interrupted by the 60s publish deadline leaves the marker; every subsequent hourly pass re-enters RecoverParquet, hits the same long rollup reader, fails again, and skips retention/compaction/cleanup entirely. maintenanceFailures climbs forever behind a 200. There is no path that abandons or quarantines a marker that cannot be completed.
| return nil, err | ||
| } | ||
| r := &Repository{root: root, Parquet: parquetStore} | ||
| if err := r.recoverCompaction(context.Background(), func(ctx context.Context, publish func(context.Context) error) error { return publish(ctx) }); err != nil { |
There was a problem hiding this comment.
HIGH — an unloadable compaction stage makes the process unbootable (carried from 526fd87c).
Open runs recoverCompaction before cleanupCompactionArtifacts. If a staged compaction output exists but cannot be loaded — truncated spans.parquet, a torn trace.fidx, a metadata version this binary doesn't accept — recoverCompaction takes the stageExists branch, PublishReplacement's loadStoredBatch(source) fails, Open returns an error and cmd/fanout/main.go calls os.Exit(1).
Restarting repeats it: the os.RemoveAll(root/compaction) that would clear the bad stage is unreachable because it runs after recovery. Recovery is a manual rm -rf on an observability appliance whose value is being up during an incident. The same class applies to readBatchMetadata's strict Version != 2 + fatal loadBatches: one bad .batch directory and the store will not open.
| return false | ||
| // PublishParquet limits reader exclusion to the atomic directory swap. | ||
| func (d *Duck) PublishParquet(ctx context.Context, publish func(context.Context) error) error { | ||
| publishCtx, cancelPublish := context.WithTimeout(ctx, 2*defaultWriterGrace) |
There was a problem hiding this comment.
MEDIUM — publishCtx budgets the reader drain and the swap out of the same 60s (new in fbc56044).
publishCtx is created before parquetMu.LockContext(publishCtx) and then passed straight into publish(publishCtx). If the reader drain consumes 55s, the closure — which starts with p.lockPublish(ctx) — receives a context with ~5s (or already expired) and can fail immediately even though publishGate is free. The exclusive parquetMu hold is then spent for nothing and the whole maintenance pass errors, feeding the latch above.
The swap's budget should start when the lock is acquired (a fresh context.WithTimeout after LockContext succeeds), not be whatever is left over from the wait.
| prepared.traces.path = filepath.Join(final, "trace.fidx") | ||
| } | ||
|
|
||
| if err := p.lockPublish(context.Background()); err != nil { |
There was a problem hiding this comment.
MEDIUM — the ingest hot path is the only publishGate waiter with no deadline (fbc56044).
CommitBatch calls p.lockPublish(context.Background()) here (and at line 158). Every other acquirer — PruneBefore, PublishReplacement, RestoreRetiredInputs — now passes a real context, so the one participant that can convert a stall into permanent data loss is the one that waits forever.
publishGate is held by maintenance publications across up to 64 os.Rename calls plus a syncDirectory of a directory that grows with the batch count. On a degraded disk that hold is unbounded, Submit blocks, the OTLP client times out and gives up, and the rows are gone. CommitBatch should take the submitting request's context (or a bounded one) so the writer can surface backpressure instead of hanging.
| return err | ||
| } | ||
| defer p.unlockPublish() | ||
| p.mu.Lock() |
There was a problem hiding this comment.
MEDIUM — p.mu (the batches-map lock) is held across up to 64 renames plus an fsync, and the ingest path takes it (carried from 895c238a).
PruneBefore takes p.mu.Lock() for the whole retire loop; PublishReplacement (line ~722) does the same. CommitBatch's very first action is p.hasBatch(metadata.ID) -> p.mu.RLock().
So ingest commits block on maintenance filesystem work through a second, undeadlined serialization point that is invisible to the publishGate/parquetMu accounting. syncDirectory(p.batchesDir) inside that window costs more as the directory grows, which is exactly the state the pass is trying to fix. The map mutation should be separated from the filesystem work (do the renames, then take p.mu only to swap map entries).
| return nil | ||
| } | ||
| if info, err := os.Stat(final); err == nil && info.IsDir() { | ||
| if err := p.lockPublish(context.Background()); err != nil { |
There was a problem hiding this comment.
MEDIUM — the one path where validation was not hoisted out of the critical section (fbc56044).
The commit message says validation moved outside the publish lock, and on the main path it did (loadStoredBatch(stage) now runs before lockPublish). But this branch — and the rename-failed branch at line 260 — still call registerBatch(final) inside the gate, and registerBatch -> loadStoredBatch opens every signal's Parquet footer and walks the entire trace.fidx validating strict ordering and contiguity. For a compacted batch that is a full scan of a 3.2M-entry index while the ingest path is blocked behind the gate.
Either hoist these loads the way the main path was, or drop the branch: with p.batches populated from disk at open and prune/compaction renaming directories away, final existing while hasBatch is false is not reachable at runtime.
| EndpointDisabledStateKey = "endpoint_rollup_v1_disabled" | ||
| defaultDuckDBPoolSize = 1 | ||
| parquetMaintenanceBatchLimit = 64 | ||
| parquetMaintenancePhaseBudget = 10 * time.Minute |
There was a problem hiding this comment.
MEDIUM — the phase budget does not bound the phase, and the reclaim rate is still below the creation rate.
Replacing parquetMaintenancePublishLimit = 4 with a 10-minute budget fixes retention (a prune publication retires 64 directories cheaply and the loop can run thousands of times). It does not fix compaction, and the two constants are not mutually consistent:
- The budget is only checked between
CompactParquetcalls (compaction.go:180), so one unbounded DuckDBCOPYover 64x50k rows plusPrepareReplacement's full re-read of that output can outrun the 1hMaintenanceIntervalon its own. "10 minute budget" names something the code does not enforce. - Each publication reclaims at most 64 directories and costs a write and a read of up to 3.2M rows. In a 10-minute window that is on the order of 10^1-10^2 publications, i.e. low thousands of directories per hour, against a creation rate of one directory per OTLP export request that
enqueueSubmissionscould not group (at 32k rows/s with typical batch sizes that is 10^3-10^5 per hour).
Everything scales with that directory count: DuckDB re-expands the *.batch glob and opens every footer on every query, updateParquetStats stats 3 files per batch on every rollup tick (60s), Trace opens every batch's .fidx, and CommitBatch fsyncs batchesDir on the ingest path. The cap moved from a count to a clock but the ceiling is still below the floor.
| for { | ||
| count, err := r.CompactParquet(ctx, compactor, maxBatches) | ||
| total += count | ||
| if err != nil || count == 0 || !time.Now().Before(deadline) { |
There was a problem hiding this comment.
MEDIUM — budget checked only after a completed compaction, with no bound on a single one.
The intent ("slow compactions finish instead of being canceled and retried forever") is right, but the result is that the storage phase has no upper bound at all: ctx is the process context, MergeParquet is an unbounded COPY, and PrepareReplacement re-reads the entire output. A pathological group can hold the maintenance goroutine past the next hourly tick.
The missing piece is an admission decision rather than a cancellation: size the group from the estimated row count so a single compaction is expected to fit the remaining budget, then let it run to completion uncancelled. As written, budget and maxBatches are two constants that cannot see each other's cost.
| var cleanupErr error | ||
| for _, entry := range entries { | ||
| name := entry.Name() | ||
| if !entry.IsDir() || strings.HasSuffix(name, telemetry.BatchSuffix) || !strings.Contains(name, ".retired") { |
There was a problem hiding this comment.
LOW-MEDIUM — retired-directory cleanup depends on a caller ordering the type cannot enforce.
Moving cleanup into Repository is the right ownership call. But cleanupRetired deletes any directory whose name merely contains .retired, including the <id>.retired-<output> directories that RestoreRetiredInputs needs to roll a compaction back. The only thing keeping that safe is that runRepositoryMaintenance happens to call RecoverParquet before CleanupParquet — and CleanupParquet is exported, so nothing stops a future caller from inverting it and destroying the rollback set for a live marker.
Make the invariant structural: read COMPACTION.json inside cleanupRetired and skip any suffix matching a live marker's output ID, rather than documenting the obligation in a comment.
Summary
The exploratory storage comparisons and benchmark POCs remain local under the ignored
experiments/directory.Breaking change
Existing DuckLake, WAL, and segment telemetry is not migrated. Deployments must start with a clean
storage.data_dir. No compatibility or fallback storage path is included.Verification
just checkand pre-push gate