WIP: CNTRLPLANE-3461: refactor defrag to minimize database lock time#378
Open
dusk125 wants to merge 8 commits into
Open
WIP: CNTRLPLANE-3461: refactor defrag to minimize database lock time#378dusk125 wants to merge 8 commits into
dusk125 wants to merge 8 commits into
Conversation
Split the monolithic defrag operation into three phases to allow writes to continue during the slow bulk copy: Phase 1 (unlocked): Commit pending writes, take a bbolt MVCC snapshot, install a write journal on batchTxBuffered, then unlock. The bulk copy runs against the snapshot while normal writes continue and are captured by the journal. Phase 2 (locked): Lock batchTx, drain the journal, replay delta writes into the temp DB. This is fast since it's only the writes that occurred during the copy. Phase 3 (locked): Atomic switchover — close old DB, rename temp file, reopen, create new transactions, unlock. Write blocking duration changes from O(db_size) to O(writes_during_copy) + fixed overhead for close/rename/reopen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a journaling-based, three-phase BoltDB defragmentation: snapshot/copy to a temp DB while allowing writes, record concurrent mutations into an in-memory defrag journal, drain and replay the journal into the temp DB, then atomically switch to the compacted DB. Adds journal types, wiring into batch transactions, and concurrency tests. ChangesDefragmentation journaling & switchover
sequenceDiagram
participant Caller as Caller (Defrag initiator)
participant Backend as Backend
participant BatchTx as BatchTx
participant Journal as Journal
participant TempDB as Temp DB
participant SourceDB as Source DB
rect rgba(100, 150, 200, 0.5)
Note over Backend,TempDB: Phase 1: Snapshot & Copy
Caller->>Backend: Defrag()
Backend->>Backend: Open TempDB (Options.Mlock=false)
Backend->>SourceDB: Commit pending writes, take snapshot tx
Backend->>BatchTx: Install defragJournal
Backend->>Backend: defragFromTx(snapshot, tmpdb, limit)
Backend->>SourceDB: Iterate buckets from snapshot tx
Backend->>TempDB: Create buckets, copy key/value pairs (periodic commits)
end
rect rgba(150, 100, 200, 0.5)
Note over Backend,TempDB: Phase 2: Concurrent Writes & Replay
par Concurrent writes
Caller->>BatchTx: UnsafePut/UnsafeDelete (during defrag)
BatchTx->>Journal: Append ops to journal
and Journal replay
Backend->>BatchTx: Lock writes
Backend->>Journal: Drain accumulated operations
Backend->>Backend: replayJournal(tmpdb, ops, limit)
Backend->>TempDB: Apply batched creates/puts/deletes (ignore missing-bucket delete errors)
end
end
rect rgba(200, 150, 100, 0.5)
Note over Backend,SourceDB: Phase 3: Atomic Switchover
Backend->>Backend: Lock tx/db paths for switchover
Backend->>TempDB: Unsafe commit
Backend->>SourceDB: Close & rename DB files
Backend->>TempDB: Reopen as primary DB
Backend->>BatchTx: Reset transaction state
Backend->>Caller: Return (defrag complete)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dusk125 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/storage/backend/backend.go (1)
540-541: ⚡ Quick winPotential resource leak if
defragFromTxpanics.If
defragFromTxpanics at line 540, thesnapTxon line 524 is never rolled back, and the cleanup in lines 544-551 is not reached. While Go's bbolt will eventually clean up on backend close, consider wrapping with a deferred rollback:if snapErr != nil { b.batchTx.Unlock() tmpdb.Close() os.Remove(tdbp) return fmt.Errorf("failed to begin snapshot tx for defrag: %w", snapErr) } + defer snapTx.Rollback() // no-op if already rolled back journal := newDefragJournal() b.batchTx.defragJournal = journal b.batchTx.Unlock() b.lg.Info("defrag: copying data (writes unlocked)") // gofail: var defragBeforeCopy struct{} err = defragFromTx(snapTx, tmpdb, defragLimit) - snapTx.Rollback()This ensures cleanup even if defragFromTx panics (though etcd typically uses Fatal on critical errors, making this a defensive safeguard).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/backend.go` around lines 540 - 541, The call to defragFromTx may panic, which would skip the explicit snapTx.Rollback() and cause a resource leak; immediately defer snapTx.Rollback() right after snapTx is created (before calling defragFromTx) so the transaction is rolled back even if defragFromTx panics, and keep the existing explicit rollback/commit/error handling logic around defragFromTx (or check/ignore the rollback error if necessary) to preserve current behavior for tmpdb and defragLimit.server/storage/backend/batch_tx.go (1)
389-394: 💤 Low valueVerify journal ordering: append happens after the operation.
The journal records the operation after
t.batchTx.UnsafeCreateBucket(bucket)completes. If the underlying operation can fail and log fatal (line 118-122 in batchTx), the journal append is never reached. This is correct behavior for the current implementation, but worth documenting.The same pattern applies to all other unsafe operations (Put, Delete, DeleteBucket).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/batch_tx.go` around lines 389 - 394, The journal append in UnsafeCreateBucket currently occurs after calling t.batchTx.UnsafeCreateBucket(bucket) which means if the underlying operation fails fatally the journal won't record it; add a brief comment above UnsafeCreateBucket explaining this ordering and rationale (i.e., we intentionally append to defragJournal only after the operation succeeds to avoid recording failed operations), and add the same clarifying comment to the other unsafe methods (UnsafePut, UnsafeDelete, UnsafeDeleteBucket) referencing the calls t.batchTx.UnsafeCreateBucket / UnsafePut / UnsafeDelete / UnsafeDeleteBucket and the corresponding j.appendCreateBucket / j.appendPut / j.appendDelete / j.appendDeleteBucket to make the behavior explicit for future readers.server/storage/backend/defrag_journal.go (1)
101-107: 💤 Low valueConsider preserving slice capacity in
drain().Setting
j.ops = nilon line 105 discards the allocated capacity. Ifdrain()is called repeatedly during defrag replay batching, this forces reallocation. Consider:func (j *defragJournal) drain() []defragJournalOp { j.mu.Lock() defer j.mu.Unlock() ops := j.ops - j.ops = nil + j.ops = j.ops[:0] return ops }This retains the underlying capacity for the next append batch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/defrag_journal.go` around lines 101 - 107, The drain() implementation drops the underlying capacity by setting j.ops = nil; change it to reset the slice length without releasing capacity (use a slice reset like j.ops = j.ops[:0]) so repeated drain() calls reuse the allocated backing array; update the defragJournal.drain method to return the current ops slice content while preserving capacity for subsequent appends (reference: defragJournal.drain and the j.ops slice).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/storage/backend/backend.go`:
- Around line 540-541: The call to defragFromTx may panic, which would skip the
explicit snapTx.Rollback() and cause a resource leak; immediately defer
snapTx.Rollback() right after snapTx is created (before calling defragFromTx) so
the transaction is rolled back even if defragFromTx panics, and keep the
existing explicit rollback/commit/error handling logic around defragFromTx (or
check/ignore the rollback error if necessary) to preserve current behavior for
tmpdb and defragLimit.
In `@server/storage/backend/batch_tx.go`:
- Around line 389-394: The journal append in UnsafeCreateBucket currently occurs
after calling t.batchTx.UnsafeCreateBucket(bucket) which means if the underlying
operation fails fatally the journal won't record it; add a brief comment above
UnsafeCreateBucket explaining this ordering and rationale (i.e., we
intentionally append to defragJournal only after the operation succeeds to avoid
recording failed operations), and add the same clarifying comment to the other
unsafe methods (UnsafePut, UnsafeDelete, UnsafeDeleteBucket) referencing the
calls t.batchTx.UnsafeCreateBucket / UnsafePut / UnsafeDelete /
UnsafeDeleteBucket and the corresponding j.appendCreateBucket / j.appendPut /
j.appendDelete / j.appendDeleteBucket to make the behavior explicit for future
readers.
In `@server/storage/backend/defrag_journal.go`:
- Around line 101-107: The drain() implementation drops the underlying capacity
by setting j.ops = nil; change it to reset the slice length without releasing
capacity (use a slice reset like j.ops = j.ops[:0]) so repeated drain() calls
reuse the allocated backing array; update the defragJournal.drain method to
return the current ops slice content while preserving capacity for subsequent
appends (reference: defragJournal.drain and the j.ops slice).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bcd07fa1-6ba1-4075-a8e0-cdb9b4036291
📒 Files selected for processing (5)
server/storage/backend/backend.goserver/storage/backend/backend_test.goserver/storage/backend/batch_tx.goserver/storage/backend/defrag_journal.goserver/storage/backend/defrag_journal_test.go
dusk125
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dusk125: This pull request references CNTRLPLANE-3461 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Add tests that validate the non-blocking defrag implementation: - TestBackendDefragConcurrentReads: reads work throughout defrag - TestBackendDefragWriteAvailability: writes stay unblocked during copy - TestBackendDefragConcurrentReadsAndWrites: simultaneous readers/writers - TestBackendDefragLogicalConsistency: hash + key-by-key verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- TestBackendDefragOverwriteDuringCopy: key overwritten during copy gets the new value after replay - TestBackendDefragNewBucketDuringCopy: bucket created during copy is present after defrag with its data - TestBackendDefragMultipleSequential: 3 sequential defrags with mutations between each, hash verified after every round - TestBackendDefragEmptyDatabase: defrag on an empty db is a no-op - TestBackendDefragLargeJournalReplay: enough writes during copy to exercise the batched commit path in replayJournal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/storage/backend/backend_test.go (2)
649-657: 💤 Low valueVerify deleted keys are gone after defrag.
The test only checks that keys 50-99 survived but doesn't verify that concurrently deleted keys (0-49) are actually removed. This would strengthen confidence that journal replay correctly handles deletes during defrag.
Suggested addition after line 657
for i := 50; i < 100; i++ { keys, _ := rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("key_%04d", i)), nil, 0) require.Lenf(t, keys, 1, "key_%04d should exist", i) } + // verify deleted keys are gone + for i := 0; i < 50; i++ { + keys, _ := rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("key_%04d", i)), nil, 0) + assert.Emptyf(t, keys, "key_%04d should be deleted", i) + } rtx.Unlock()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/backend_test.go` around lines 649 - 657, Add assertions after the defrag verification to ensure keys 0-49 were actually deleted: after calling b.ForceCommit() and obtaining rtx := b.BatchTx() (and rtx.Lock()), iterate i from 0 to 49 and use rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("key_%04d", i)), nil, 0) to assert the returned keys slice is empty (or length 0) for each deleted key; then unlock rtx. This complements the existing checks for keys 50-99 and verifies deletes survived defrag/journal replay.
965-974: ⚡ Quick winStrengthen assertion to verify all journal writes are present.
The test aims to exercise the batched commit path in
replayJournal, but only asserts that "at least some" journal keys exist. Since writes during defrag should be captured by the journal and replayed into the new DB, allwrittenkeys should be present. The current assertion would pass even if most keys were lost.Suggested change
// verify at least some journal keys exist var found int for i := 0; i < written; i++ { keys, _ := rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("journal_%06d", i)), nil, 0) if len(keys) > 0 { found++ } } rtx.Unlock() - assert.Greater(t, found, 0, "journal writes should be present after defrag") + assert.Equal(t, written, found, "all journal writes should be present after defrag")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/backend_test.go` around lines 965 - 974, The test currently only checks that some journal entries exist after defrag; change it to assert that every expected journal key written during the test is present. Use the existing loop that calls rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("journal_%06d", i)), nil, 0) for i in 0..written-1 and count missing keys (or increment a present counter) and then assert that present == written (or missing == 0). This ensures replayJournal/defrag behavior is validated for all written keys rather than just >0; keep the same identifiers (rtx.UnsafeRange, schema.Test, journal_%06d, written) when locating and updating the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/storage/backend/backend_test.go`:
- Around line 649-657: Add assertions after the defrag verification to ensure
keys 0-49 were actually deleted: after calling b.ForceCommit() and obtaining rtx
:= b.BatchTx() (and rtx.Lock()), iterate i from 0 to 49 and use
rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("key_%04d", i)), nil, 0) to
assert the returned keys slice is empty (or length 0) for each deleted key; then
unlock rtx. This complements the existing checks for keys 50-99 and verifies
deletes survived defrag/journal replay.
- Around line 965-974: The test currently only checks that some journal entries
exist after defrag; change it to assert that every expected journal key written
during the test is present. Use the existing loop that calls
rtx.UnsafeRange(schema.Test, []byte(fmt.Sprintf("journal_%06d", i)), nil, 0) for
i in 0..written-1 and count missing keys (or increment a present counter) and
then assert that present == written (or missing == 0). This ensures
replayJournal/defrag behavior is validated for all written keys rather than just
>0; keep the same identifiers (rtx.UnsafeRange, schema.Test, journal_%06d,
written) when locating and updating the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ebe9587a-2ca3-482e-b145-ec5431475c50
📒 Files selected for processing (1)
server/storage/backend/backend_test.go
If a panic occurs after batchTx.tx is set to nil during the Phase 3 switchover, inflight requests could use the invalid tx and produce secondary panics that shadow the real cause. Restore the deferred recover that catches the original panic and calls Fatal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
During defrag, writes to the old database are wasted I/O since it is about to be replaced. When the defrag journal is active, mutation methods on batchTxBuffered now write to the journal and in-memory buffer only, skipping the bbolt write. Commits are also skipped during defrag to prevent readTx.buf from being cleared — since puts are not in bbolt, clearing the read buffer would make them invisible. This addresses feedback to decouple transaction buffering from bbolt: writes are buffered on the etcd side and handed over as a batch to the new database via journal replay, without writing to a database that is about to be replaced. Adds three new tests: - TestBackendDefragReadsDuringDefragSeeBufferedPuts: verifies ConcurrentReadTx sees puts made during defrag via the buffer - TestBackendDefragExceedBatchLimit: writes exceed batchLimit during defrag, exercising the counter-reset path in Unlock - TestBackendDefragCommitDuringDefragPreservesData: ForceCommit during defrag does not clear buffered data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/storage/backend/backend_test.go (2)
535-539: 💤 Low valueConsider aligning the threshold with the documented expectation.
The comment states latency should be "well under a second" but the assertion allows up to 5 seconds. This generous threshold likely exists for CI stability, but the discrepancy could mask real performance regressions. Consider either lowering the threshold (with appropriate CI tolerance) or updating the comment to reflect the actual bound.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/backend_test.go` around lines 535 - 539, The test's comment claims "well under a second" but the assertion uses 5*time.Second; update the test to make these consistent by either lowering the assertion bound on maxWriteLatency in the assert.Less call to 1*time.Second (or another tighter CI-approved threshold) or change the comment to reflect the 5-second bound; modify the assert.Less invocation and/or the preceding comment near the maxWriteLatency assertion so the documented expectation and the actual timeout match.
1000-1001: ⚖️ Poor tradeoffTiming-based synchronization may cause test flakiness.
The
time.Sleep(10 * time.Millisecond)to wait for defrag to start is timing-dependent and could cause flakiness on slow CI machines or under resource contention. Consider exposing a hook or using a synchronization primitive from the backend to signal when defrag has entered the copy phase, if such a mechanism is feasible without overly complicating the production code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/storage/backend/backend_test.go` around lines 1000 - 1001, The test uses a timing-based wait (time.Sleep(10 * time.Millisecond)) to assume defrag has started; replace this with a deterministic synchronization point by adding a test hook or synchronization primitive in the backend that signals when defrag enters the copy phase (e.g., a channel or condition variable exposed as a test-only field or a method like WaitForDefragCopy/DefragStarted channel) and have backend_test.go wait on that signal instead of sleeping; update the defrag code paths (the goroutine that begins copy work) to close/send on that hook when entering the copy phase and in backend_test.go replace the Sleep with receiving from the hook with a reasonable timeout to avoid hangs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/storage/backend/backend_test.go`:
- Around line 535-539: The test's comment claims "well under a second" but the
assertion uses 5*time.Second; update the test to make these consistent by either
lowering the assertion bound on maxWriteLatency in the assert.Less call to
1*time.Second (or another tighter CI-approved threshold) or change the comment
to reflect the 5-second bound; modify the assert.Less invocation and/or the
preceding comment near the maxWriteLatency assertion so the documented
expectation and the actual timeout match.
- Around line 1000-1001: The test uses a timing-based wait (time.Sleep(10 *
time.Millisecond)) to assume defrag has started; replace this with a
deterministic synchronization point by adding a test hook or synchronization
primitive in the backend that signals when defrag enters the copy phase (e.g., a
channel or condition variable exposed as a test-only field or a method like
WaitForDefragCopy/DefragStarted channel) and have backend_test.go wait on that
signal instead of sleeping; update the defrag code paths (the goroutine that
begins copy work) to close/send on that hook when entering the copy phase and in
backend_test.go replace the Sleep with receiving from the hook with a reasonable
timeout to avoid hangs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ecb071bb-e474-42cb-9c92-c829a8856968
📒 Files selected for processing (2)
server/storage/backend/backend_test.goserver/storage/backend/batch_tx.go
Tests that readers holding ConcurrentReadTx snapshots from the old database are not disrupted when Phase 3 closes and replaces the DB: - TestBackendDefragConcurrentReadTxSurvivesSwitchover: a single ConcurrentReadTx acquired before defrag is held across the entire switchover, verifying that Phase 3 blocks on db.Close() until the reader releases, and that reads on the old snapshot work throughout - TestBackendDefragMultipleConcurrentReadTxDuringSwitchover: five concurrent readers continuously create and release ConcurrentReadTx during defrag, exercising the txWg synchronization across the Phase 3 boundary Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/storage/backend/backend_test.go`:
- Around line 375-399: Make the test deterministic by gating the concurrent
writer on an explicit "defrag in progress" signal and asserting at least one
write happened; add a defragStarted := make(chan struct{}) and an atomic counter
(e.g., var opsDone int32) that the writer increments after each UnsafePut inside
the goroutine, have the writer wait for <-defragStarted before starting the
loop, start Defrag in a separate goroutine and close(defragStarted) immediately
before launching it so the writer runs during Defrag, then after Defrag returns
close(stop), wg.Wait(), and require.Greater(t, atomic.LoadInt32(&opsDone),
int32(0)) to ensure at least one operation occurred during the defrag run
(referencing b.Defrag(), b.BatchTx(), tx.UnsafePut and the stop/wg
synchronization).
- Around line 924-955: The test only logs how many journal ops were written
during Defrag but doesn't assert that the threshold was crossed, so add an
assertion after reading written (after b.Defrag(), wg.Wait(), written :=
<-totalWritten) that verifies written meets or exceeds DefragLimitForTest()
(e.g. require.GreaterOrEqual(t, written, DefragLimitForTest())) to ensure the
batched replay path in replayJournal is actually exercised; reference the test's
use of b.Defrag(), the written variable, and DefragLimitForTest() when adding
this check.
- Around line 1204-1217: The test currently sleeps 50ms but doesn't prove
Defrag() is actually blocked by the old reader; add a non-blocking receive on
the defragDone channel before calling longReader.RUnlock() to assert it's still
pending. Specifically, before longReader.RUnlock(), perform a select { case err
:= <-defragDone: require.FailNowf(t, "Defrag completed early", "err=%v", err)
default: } (or equivalent using require/assert) to fail if defragDone already
delivered; keep the rest of the test (longReader.UnsafeRange, RUnlock, then
receive from defragDone) unchanged so the test now verifies Defrag() is blocked
by longReader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54bf14cd-7e69-440f-aa8b-c5756bc82e8f
📒 Files selected for processing (1)
server/storage/backend/backend_test.go
Address CodeRabbit review feedback: - Add atomic counter to TestBackendDefragConcurrentWrites to assert writes actually occurred during defrag - Increase pre-populated data in TestBackendDefragLargeJournalReplay and assert written ops exceed defragLimit to ensure batched replay path is exercised - Add non-blocking select in TestBackendDefragConcurrentReadTxSurvives Switchover to prove defrag is blocked by the outstanding reader Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TestBackendDefragConcurrentWrites: gate writer on defragStarted channel so it only runs during defrag, increase pre-populated data to 5000x1KB keys to ensure Phase 1 takes long enough for the writer to execute. TestBackendDefragExceedBatchLimit: increase pre-populated data from 1 seed key to 5000x1KB keys and batch 10 puts per lock cycle. On CI, the single seed key made Phase 1 trivially fast — only 56 keys were written, failing the >100 assertion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@dusk125: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split the monolithic defrag operation into three phases to allow writes to continue during the slow bulk copy:
Phase 1 (unlocked): Commit pending writes, take a bbolt MVCC snapshot, install a write journal on batchTxBuffered, then unlock. The bulk copy runs against the snapshot while normal writes continue and are captured by the journal.
Phase 2 (locked): Lock batchTx, drain the journal, replay delta writes into the temp DB. This is fast since it's only the writes that occurred during the copy.
Phase 3 (locked): Atomic switchover — close old DB, rename temp file, reopen, create new transactions, unlock.
Write blocking duration changes from O(db_size) to O(writes_during_copy) + fixed overhead for close/rename/reopen.
Summary by CodeRabbit
New Features
Bug Fixes
Tests