Guard both the direct read path and the peer/commit read-ahead readers against
a concurrent write_at truncation: executeReadPlan and getReadPlan now stamp
and re-check a truncation epoch, discarding a plan that straddled a rewrite
instead of serving stale-but-valid old records. Reject reads from compressed
changelog files with NOT_IMPLEMENTED (positions are decompressed-stream
offsets and cannot be seeked on the raw file), and split file_mutex into
separate withReadLock/withWriteLock so concurrent readers of the same flushed
file no longer serialize behind each other.
Unify PerPeerReader into ReadAheadReader, shared by both the per-peer
catch-up path and the always-on commit reader, and always plan read-ahead
cursors up to max_index_with_location rather than clipping to
latest_logs_cache's boundary, since that cache is a size-bounded sliding
window that can move independently of what's persisted.
Rename the keeper_log_readahead_* coordination settings to log_readahead_*
and replace commit_logs_cache_size_threshold/commit_logs_cache_entry_count_threshold
(now OBSOLETE) with log_readahead_commit_window_bytes, validated so
log_readahead_pool_threads is either 0 (auto) or >= log_readahead_max_peer_readers.
Update docs and integration test configs/helpers accordingly.
Add new gtest coverage for the epoch race, compaction races, N+1 file
planning, and disabled-read-ahead modes; convert existing sub-case comments
to SCOPED_TRACE for clearer failure output.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Step 1 of decoupling the Coordination gtests from KeeperMemoryStorage
internals: replace the typed-test fixture (TestParam<TStorage, compress>)
with a value-parametrized one. TestParam is now a plain struct with a
non-constexpr enable_compression field, CoordinationTest derives from
testing::TestWithParam<TestParam>, all TYPED_TEST become TEST_P, and a
single INSTANTIATE_TEST_SUITE_P lives in gtest_coordination_common.cpp.
A makeKeeperContext helper centralizes KeeperContext creation so a future
TestParam field selecting the KeeperStorage implementation can be applied
in one place.
Storage usages temporarily point at KeeperMemoryStorage; subsequent
commits replace them with KeeperStorage::create and virtual-method access.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comprehensive simplification of the per-peer decoded read-ahead subsystem
introduced alongside the Keeper changelog read-ahead feature. All changes
are behavior-preserving unless noted.
Lifecycle and state:
- Replace three overlapping "reader finished" signals (bool closed,
ReaderTerminalState, std::atomic<bool> fill_done) with a single
ReaderState enum {Running, Exhausted, Compacted, Error, Closed}.
shutdown() now joins fill tasks via pool->wait() instead of per-reader
fill_done.wait().
- Move PerPeerReader struct and ReaderState enum from Changelog.h into
Changelog.cpp (forward declaration only in header).
Dead code and hot-path fixes:
- Delete unreachable getLogEntriesBetween overloads (~110 lines) that
carried the original prefetch-wedge bug.
- Collapse the triple-discard in serveReadAhead's cache-item branch to a
single discardBeforeLocked(current_index + 1) call.
Function decomposition:
- Break ~250-line serveReadAhead into ensureReadAheadPoolLocked,
evictIdleReadersLocked (now O(n) with a size/time gate instead of O(n²)
unconditional scan), acquireReaderLocked, installPlanLocked, drainReader.
- Break ~370-line fillTask into file-scope helpers: ensureOpenAt (returns
OpenResult enum replacing three out-booleans), decodeChunk, appendChunk,
fillFromCursor (returns CursorOutcome {Eof, Restart, Parked, Terminal}).
Resume cursor:
- Consolidate six scattered setResumeCursorLocked calls to a single
canonical write at the top of each fillFromCursor iteration.
- Clear resume_cursor when installing new pending cursors (superseded).
- ensureOpenAt no longer writes resume_cursor.
Naming and conventions:
- Rename PerPeerReader fields: deque→decoded_entries, deque_front_index→
decoded_front_index, deque_bytes→decoded_bytes, cv→fill_serve_cv.
- Add entryBytes() helper; use popFrontLocked() in discardBeforeLocked.
- Convert anonymous-namespace helpers to PerPeerReader methods with
TSA_REQUIRES(deque_mutex) on Locked variants; add generationChangedLocked
to centralize generation comparisons.
- Rename complete→sealed_for_read_ahead; add markSealedForReadAhead().
- Rename expected_entry_count→requested_entry_count.
Type system:
- Unify LogReadPlan::FileRun and FillCursor into FileSpan {file_description,
position, first_index, count}. count=0 means read to EOF for fill cursors.
- Move fill cursors into optional<ReadAheadWindow> (alias for deque<FileSpan>)
on LogReadPlan; readahead_engaged → read_ahead_window.has_value().
- Extract decodeOneRecord shared helper used by both executeReadPlan and
decodeChunk.
Settings and docs:
- Fix misleading settings descriptions for fill_timeout_ms and window_bytes.
- Fix stale .zstd comment in getReadAheadPlan.
- Remove default argument from virtual log_entries_ext; extract
buildPlanUnderLock lambda in KeeperLogStore.
Thread-safety annotations:
- Add TSA_REQUIRES(deque_mutex) to all PerPeerReader Locked methods.
- Document remaining TSA_NO_THREAD_SAFETY_ANALYSIS suppressions.
Tests:
- Remove "L2" prefix from all read-ahead test names.
- Rename ReadAheadDisabledPathIdentical→ReadAheadMatchesDirectPath; add
genuinely-disabled case (ReadAheadSettings{.enabled=false}).
- Rename ReadAheadByteHintTruncation→DirectPathByteHintTruncation.
- Fix reader.detach() → reader.join() in ReadAheadShutdownJoinsFills.
- Fix missing ReadAheadSettings{} in gtest_coordination.cpp and
gtest_coordination_snapshot.cpp constructors.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Review-fix pass over the keeper-snapshot-locking branch (post master merge).
- G-F1: startup-retained duplicate same-index snapshot files were only logged, never
tracked, so detachOutdatedSnapshotsIfNeeded could never reclaim them — they leaked on
disk until the next restart. Track them in retained_duplicate_snapshots (kept
retained-window duplicates and re-point orphans) and retire them together with their
index. Aged out without a restart; new test RetainedDuplicateAgesOutWithoutRestart.
- detachSnapshotForRemoval now returns the detached pins (registered entry + tracked
duplicates) instead of unlinking in place, so the create path's unlink happens in
Phase 4 outside snapshots_lock (no disk I/O under the lock).
- C-F1: snapshots_queue.push can throw bad_alloc (emplaceImpl allocates before the
noexcept move); wrap it and route both the false-return and throw outcomes to the same
inline cleanup so when_done(false) fires exactly once.
- G-F2: keeper-utils snapshot-analyzer deserialized by parsed index, so with same-index
duplicates it could analyze a different file than the one named; deserialize the exact
named path instead.
- C-F4: corrected the create skip-branch log wording (condition is >=, not "bigger").
- retained_duplicate_snapshots is an unordered_map (point lookups only).
C-T1: the SameIndexCreateFailsClosedOnMissingRegisteredFile test was deliberately removed
(it asserted a fail-closed existsFile probe the birth-unique-names design no longer has — a
registered same-index entry is pinned by protected_pending_snapshot_log_idx so retention
cannot unlink it before apply); the adopt site documents this in a comment.
Known untested corners (pre-accepted, not regressions): the cross-disk re-point orphan path
and the create-path Phase-4 deferred unlink (gtest covers the synchronous removeSnapshot
path), the keeper-utils analyzer (no gtest harness), the push bad_alloc leg, and the
deferred receive-path I/O under snapshots_lock (multi-disk only).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Snapshot files are now created with birth-unique names
(`snapshot_<idx>_<uuid>.bin[.zstd]`), which removes same-path write races entirely:
local snapshot creation writes outside `snapshots_lock` and publishes metadata under
it (adopting a concurrently published snapshot instead of overwriting it), so
`read_logical_snp_obj` no longer waits behind a full snapshot write.
`apply_snapshot` no longer blocks on in-flight create tasks
(`KeeperStateMachine::storage` is a `shared_ptr` captured by the task), so no
claim/deferral machinery is needed. Startup keeps duplicate files for retained
indexes as redundant recovery points and deletes duplicates outside the retention
window. S3 keys stay legacy-shaped via a canonical name mapping.
Also fix failed snapshot-queue-push cleanup so `when_done(false)` is invoked exactly
once, and make the `KeeperStorageSnapshot` constructor exception-safe.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split request processing (batching, session management, request/response
threads) into a separate `KeeperRequestDispatcher` class, leaving
`KeeperDispatcher` as a thin shell for snapshots, session ID assignment,
session cleanup, and cluster configuration management.
Key interface changes:
- `KeeperStateMachine` now takes a `KeeperResponseCallback` instead of
a `ResponsesQueue &`, decoupling the state machine from the queue type.
- `KeeperServer` constructor takes `KeeperResponseCallback` instead of
`ResponsesQueue &`.
- `KeeperServer::KeeperRaftServer` moved from .cpp anonymous definition
to the header for visibility.
- `ZooKeeperResponseCallback` moved to `KeeperCommon.h`, returns `bool`
for future flow control.
- `shutdown` now takes a `closed_all_connections` parameter.
- Consolidated `finishSession` calls into a single `SCOPE_EXIT` in
`KeeperTCPHandler`.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `static` to file-local functions and forward declarations
for cross-TU functions to satisfy `-Wmissing-prototypes`.
This ensures all functions have visible prototypes before their
definitions, improving code quality and catching accidental
linkage issues.
Reference: https://github.com/ClickHouse/ClickHouse/issues/76944
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
