Repository navigation
fix(reconciler): retry failed batcher flushes with bounded backoff - #1200
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism with exponential backoff and a retry budget for transient storage failures in the UpdateBatcher component. It also refactors permanent storage error detection into a shared package, updates documentation, and adds comprehensive unit tests. The review feedback focuses on performance optimizations: specifically, avoiding unnecessary heap allocations in updatePointers by directly referencing slice elements, and replacing the allocating groupByWorkUnit helper with a zero-allocation countUniqueWorkUnits function since it is only used for metrics observation.
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism for storage updates in the JobSet reconciler. It adds a retry queue with exponential backoff for transient storage errors, while permanently failing updates are dropped. The implementation includes logic to merge pending updates with retries, handle concurrency, and provide observability via new metrics. My feedback focuses on optimizing the critical section by reducing lock contention in takeBatches and mergeRetriesLocked, suggesting the use of strconv for efficient metric formatting, and improving performance by avoiding unnecessary struct copies in updatePointers.
9668977 to
d43207e
Compare
Previously a failed UpdateWorkUnitExecutions call dropped every update in
the batch. Transient failures are now requeued per (xid, wid, source) slot
with exponential backoff (tick, min 250ms, doubling, 30s cap) and a 5m
retry budget. Newer pending updates supersede queued retries (EID-aware),
and not-yet-due retries piggyback on experiments already flushing.
A permanent failure still drops the whole batch, now counted with
reason=permanent.
- Move IsPermanentStorageError to shared for reuse by jobset and batcher.
- New metrics: batcher_updates_dropped_total{xid,reason},
batcher_updates_retried_total{xid}, batcher_retry_queue_size.
Part of b/569896494
TAG=agy
CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811
…k unit counting Address review on #1200: point directly into batch entries and count unique work units in one pass (groupByWorkUnit belongs in the isolation layer). Part of b/569896494 TAG=agy CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811
…d batches outside UpdatesMu
Share workUnitState, workUnitExecution, and testUpdate from batcher_test.go so batcher_retry_test.go does not add a duplicate legacy proto import. TAG=agy CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811
45ca2e1 to
394427c
Compare
…ction Comment link: #1200 (comment) Comment link: #1200 (comment) Comment link: #1200 (comment) TAG=agy
Summary
Retry
UpdateWorkUnitExecutionsfailures in the reconciler batcher instead of dropping them. Transient failures are re-queued for each(xid, wid, source)slot. The wait between retries starts at the tick interval (at least 250ms), doubles each time, caps at 30s, and an update is dropped after a 5-minute retry budget. Permanent failures still drop the batch, now counted in a metric. Layer 2 of the b/569896494 stack.Motivation & Context
Before this change,
processBatchesswapped out the pending map and then only logged and counted storage errors. Any failure, including the 30s timeout, silently lost every update in the batch (b/569896494 point 2).Design:
shared.IsPermanentStorageError(NotFound, InvalidArgument, FailedPrecondition, PermissionDenied) moves fromjobsettoshared, so the JobSet reconciler and the batcher use one classifier.takeSnapshotskeeps the happy path (len(b.retries) == 0) to anUpdatesMuwith zero extra allocations, and iterates onlyb.retries($O(|\text{retries}|)$) when retries are queued.buildExperimentBatchconstructs and sorts each experiment's[]batchEntryoutsideUpdatesMuin its per-experiment goroutine.Removealso evicts matchingb.retriesentries withEID <= maxEIDand records an in-flight tombstone while a flush is active so a failing in-flight RPC cannot re-queue a removed or superseded update (or falsely countretry_budget_exhausted).Key Changes
cmd/reconciler/shared/batcher_retry.go(new): retry queue, backoff, budget,takeSnapshots,buildExperimentBatch, and in-flightRemove/Addsupersede handling (removeRetryLocked,finishFlush,isSupersededLocked).cmd/reconciler/shared/storage_errors.go(new):IsPermanentStorageError.cmd/reconciler/shared/batcher.go:processBatchesusestakeSnapshots+buildExperimentBatch+finishFlush;Removealso evicts queued and in-flight retries; flushes keep each update's(wid, source)key.batcher_updates_dropped_total{xid,reason},batcher_updates_retried_total{xid}, gaugebatcher_retry_queue_size.docs/flows/reconciliation.md§6 is updated.Assumptions
xmc_actuator_batcher_update_calls_totalnow includes retries.batcher_updates_processed_totalcounts a retried update again. No dashboards or alerts in the repo reference either metric.Verification & Testing
bazel test //cmd/reconciler/...: 7/7 pass.bazel test --@io_bazel_rules_go//go/config:race //cmd/reconciler/shared:shared_test: passes.batcher_retry_test.gouses a scripted fault-injectingStorageUpdaterand a fake clock:TestRetry_RemoveEvictsQueuedRetries:Removeevicts queued retries withEID <= eid, preserves retries withEID > eid, and updatesbatcher_retry_queue_size;TestRetry_InFlightRemoveAndAddSupersede: in-flightRemove(EID >= entry.EIDvsEID < entry.EID) and in-flightAdd(EID >= entry.EIDsuppressing retry andretry_budget_exhausteddrop, andEID < entry.EIDevicted on failure);Add/flush/requeue test, run with the race detector.Related Issues / Bugs
Part of b/569896494. Stacked on #1189.
TAG=agy
CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811