[#33506] DocDB: Start tablet flush when a snapshot create operation is prepared - #24
Draft
craigsoules wants to merge 3 commits into
Draft
[#33506] DocDB: Start tablet flush when a snapshot create operation is prepared#24craigsoules wants to merge 3 commits into
craigsoules wants to merge 3 commits into
Conversation
`CREATE_ON_TABLET` snapshot operations are applied inline on the Raft apply path while the tablet's ReplicaState mutex is held. The synchronous RocksDB flush at the start of `TabletSnapshots::Create` is usually the longest part of that window on a write-heavy tablet, and while it runs, every write, lease-checked read, and follower `UpdateConsensus` on the tablet blocks behind the mutex. Kick off a non-blocking flush of all DBs from `TabletSnapshots::Prepare`, which runs on the Preparer thread of every replica before the operation is replicated, so the flush overlaps Raft replication. The synchronous flush in `Create` stays as the correctness backstop and now only waits for the delta written between prepare and apply. Also widen that apply-time flush from the regular DB to `FlushFlags::kAllDbs`: the intents DB flush previously happened inside checkpoint creation while additionally holding `create_checkpoint_lock`, where it also contends with remote bootstrap sessions. Guarded by the new runtime flag `snapshot_create_flush_on_prepare` (default true); correctness does not depend on the prepare-time flush happening or completing. Assisted-By: devx/5ad4e5f4-d16e-4701-984d-e10a814390c8 --- _automated · pi (claude-fable-5)_
On followers, `Operation::Prepare` runs inline in `UpdateConsensus` while the ReplicaState lock is held (`PreparerImpl::Submit` invokes `PrepareAndStartTask` directly for follower-side operations); only the leader prepares on the preparer thread. Even a non-waiting `Tablet::Flush` synchronously takes the RocksDB mutex, enters the write queue, and switches the memtable, so calling it inline from Prepare kept bounded-but-nonzero work under the very lock this change is meant to relieve. Dispatch the flush scheduling call to the existing per-process snapshot cleanup pool via a new concurrent-mode token. A separate token is used instead of the serial cleanup token so the preflush does not queue behind recursive snapshot directory deletions, which PITR issues around the same time as creations. The token is shut down in `CompleteShutdown`, so the task cannot outlive the tablet. Without an installed pool (before `InitTabletPeer`, i.e. before consensus traffic, and in tests) the flush is scheduled inline as before. Assisted-By: devx/5ad4e5f4-d16e-4701-984d-e10a814390c8 --- _automated · pi (claude-fable-5)_
The snapshot cleanup pool has only `snapshot_cleanup_pool_size` (4) workers shared by every tablet in the process, so concurrent recursive snapshot deletions can occupy all of them. A preflush task queued behind them may only start after the snapshot operation has already applied, providing no benefit, and since `ThreadPoolToken::Shutdown` waits for queued tasks, it could also delay tablet shutdown behind other tablets' cleanup work. Move the preflush token onto the unbounded raft pool, which `Tablet::SetCleanupPool` already receives for intent file cleanup. Also drop the inline-flush fallback on submission failure: that failure means the pool or token is shutting down, and flushing inline would reintroduce the consensus-path work this is meant to avoid. The inline fallback remains only for the no-pool case (before `InitTabletPeer` and in tests). Adds a regression test that occupies every cleanup pool worker with blocked deletions and verifies the preflush still runs. Assisted-By: devx/5ad4e5f4-d16e-4701-984d-e10a814390c8 --- _automated · pi (claude-fable-5)_
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
1 participant
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.
Summary
CREATE_ON_TABLETsnapshot operations are applied inline on the Raft apply path while the tablet's ReplicaState mutex is held. The synchronous RocksDB flush at the start ofTabletSnapshots::Createis usually the longest part of that window on a write-heavy tablet: while it runs, every write, lease-checked read, and followerUpdateConsensusfor the tablet blocks behind the mutex. Namespace-wide snapshots (backups, PITR schedules) hit every tablet at once, so one slow flush per tablet can escalate into node-wide latency spikes.This PR starts the flush earlier so the apply-time flush only has to cover the delta:
TabletSnapshots::Preparenow schedules a non-waiting flush of all DBs when aCREATE_ON_TABLEToperation enters the pending queue, overlapping the flush with Raft replication. On followers,Prepareruns inline inUpdateConsensusunder the ReplicaState lock (PreparerImpl::SubmitinvokesPrepareAndStartTaskdirectly), and even a non-waitingTablet::Flushsynchronously enters the RocksDB write queue to switch the memtable -- so the scheduling call is dispatched to the unbounded raft pool (already handed toTablet::SetCleanupPoolfor intent file cleanup) via a concurrent token that is shut down with the tablet. The bounded snapshot cleanup pool is deliberately not used: itssnapshot_cleanup_pool_size(4) process-wide workers can be fully occupied by recursive snapshot deletions, which would starve the preflush until after apply and couple tablet shutdown to other tablets' cleanup work.Createis widened from the regular DB toFlushFlags::kAllDbs, so the intents flush no longer runs inside checkpoint creation while additionally holdingcreate_checkpoint_lock(), where it contends with remote bootstrap checkpoints.Correctness does not depend on the prepare-time flush happening or completing: the synchronous flush during apply remains the backstop, and the checkpoint's hybrid-time filter continues to define transactional snapshot content. The preflush is best-effort -- dropped on executor shutdown, and scheduled inline when no pool is installed yet (before
InitTabletPeer, i.e. before the tablet serves consensus traffic).Guarded by the new runtime flag
snapshot_create_flush_on_prepare(default: true).Upgrade/Rollback safety
No wire-format, on-disk-format, or catalog changes. The new gflag
snapshot_create_flush_on_prepareis runtime-settable and defaults to true; it changes only when a RocksDB flush is scheduled, not what data a snapshot contains (each replica's checkpoint has always been a superset trimmed by the hybrid-time filter, and the apply-time flush backstop is unchanged). Behavior is node-local, so mixed-version clusters are unaffected. Rollback: set the flag to false at runtime or downgrade the binary -- no persistent state depends on it.Test plan
All run locally (release, clang21, arm64), one test per execution:
TabletSnapshotsTest.PrepareFlushesTabletForSnapshotCreation(pool dispatch path)TabletSnapshotsTest.PrepareFlushesTabletWithoutCleanupPool(inline fallback)TabletSnapshotsTest.PreflushNotStarvedByCleanupPool(every cleanup-pool worker blocked by deletions; preflush still runs)TabletSnapshotsTest.PrepareDoesNotFlushWhenDisabledTabletSnapshotsTest.PrepareDoesNotFlushForNonCreateOperationsTabletSnapshotPathTest.FlushOnPrepareEnabledByDefaultTabletSnapshotsTest.ShutdownWaitsForRunningCleanup,SharedPoolBoundsCleanupConcurrency,RetainsDeletionUntilCleanupPoolIsInstalled,DeletesSynchronouslyWhenAsyncCleanupDisabled(SetCleanupPool/CompleteShutdown changes)TestRaftGroupMetadata.TestDeleteTabletDataClearsDisk(exercises the modifiedCreateflush path)