Skip to content

[Store] Remove snapshot serializers orphaned by the legacy snapshot removal - #4774

Merged
Aionw merged 1 commit into
kvcache-ai:mainfrom
Aionw:store-remove-orphaned-snapshot-serializers
Oct 23, 2026
Merged

Aionw merged 1 commit into
kvcache-ai:mainfrom
Aionw:store-remove-orphaned-snapshot-serializers

Conversation

@Aionw

@Aionw Aionw commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Follow-up to #4406. That PR removed the Master-generated snapshot and restore path, but the serialization code only that path called is still compiled into the master. None of it has a caller outside its own tests; this PR deletes it.

Removed:

  • serialize/serializer.{h,cpp}: the msgpack Serializer<T> specializations for allocators, buffers, replicas and mounted segments, plus SerializationHelper and SerializationError.
  • ha/snapshot: StoreResourceSnapshotCodec, AllocatorSnapshotCodec, LocalSsdCodec.
  • SegmentSerializer and SegmentView (segment.h); TaskManagerSerializer together with restore_task/clear_all, which only it used; LocalSsdManager::ExportPersistedState/RestorePersistedState and LocalSsdPersistedState.
  • The detached snapshot path from [Store] Add fork-safe SegmentPool snapshot codecs #3706: SegmentPool::CaptureSnapshot/RestoreSnapshot, OffsetBufferAllocator and OffsetAllocator CaptureSnapshot/Restore with their snapshot structs and layout validation, AdoptMode::ReplacePool, IsKnownSegmentStatus.
  • The tests that only covered the above.

Kept on purpose:

  • SegmentPool::WriteAccess::PrepareAdopt, back to its pre-[Store] Add fork-safe SegmentPool snapshot codecs #3706 signature: AllocationKeepsReadLockAcrossAllocatorCall uses it to install a blocking allocator.
  • SegmentPoolTest.DestroyedPoolAndRetainedAllocatorRemoveMetricLabels, rewritten to mount directly instead of restoring a snapshot. [Store] Add fork-safe SegmentPool snapshot codecs #3706 also made the buffer allocator destructors remove their segment's metric labels and made remove_segment_metrics keep a label that still has usage. Both apply to ordinary mounts and had no other test.
  • The byte serializer in include/serializer.h (serialize_to/deserialize_from), which storage_backend.cpp still uses.
  • Error codes such as SERIALIZE_FAIL/DESERIALIZE_FAIL, which are part of the public ErrorCode enum.

The only non-deletion change is in OffsetBufferAllocator's constructor: its node cap goes back to the literal 64 * 1024 * 1024 it had before #3706 renamed it OffsetAllocatorSnapshot::kMaxNodes (same value, 1u << 26).

Overlap: #3707 (SegmentPool cutover) adapts several of these files to SegmentPool; after this lands, those hunks resolve as deletions. #4133 (stacked on #3707) removes the legacy SegmentManager itself; this PR only removes its snapshot serializer, which nothing calls on main today.

Suggested Merge Priority

  • P0 — Urgent
  • P1 — High
  • P2 — Normal
  • P3 — Low: Experimental feature, optional enhancement, refactoring, or cleanup with no urgent user impact

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Refactor

How Has This Been Tested?

Release build, BUILD_UNIT_TESTS=ON, USE_NOF=OFF.

Test commands:

cmake --build build -j14            # full build
ctest --test-dir build -j12 --timeout 300 \
  -E '^(ordered_oplog_writer_test|tcp_transport_test|transfer_metadata_test)$'
pre-commit run --files $(git diff --name-only --diff-filter=ACMR origin/main...HEAD)

Test results:

  • Unit tests pass: 243/244. The one failure, DfsSyncClientTest.DfsOnlyBatchFailureOnlyRevokesFailedKey, fails the same way on unmodified origin/main.
  • The full build has two failures that also happen on origin/main and are unrelated: tests/e2e/chaosctl.cpp (sleep undeclared) and ordered_oplog_writer_test (libaio link). The three excluded tests need a network/RDMA environment.
  • cmake-format was skipped: it rewrites unrelated, pre-existing lines in tests/CMakeLists.txt; this PR only deletes lines there.

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh (clang-format 20 on changed lines)
  • I have run pre-commit on the files changed in this PR and all hooks pass (except cmake-format, see above)
  • I have updated the documentation (if applicable) — no docs reference the removed code
  • I have added tests to prove my changes are effective — removal only
  • For changes >500 LOC: I have filed an RFC issue — ~5.9k lines, all deletions of code with no production caller; no behavior change

AI Assistance Disclosure

  • AI tools were used (specify below)

Claude Code traced the callers, made the deletions and ran the build and tests. The human submitter is responsible for reviewing every changed line and defending the change.

@Icedcoco

Copy link
Copy Markdown
Collaborator

I noticed that serializer_test.cpp also includes tests for the generic byte serializer in include/serializer.h, which remains in use by storage_backend.cpp. Would it be worth considering whether those tests should be kept in a separate target, since they cover code that this PR leaves in place?

@Aionw
Aionw force-pushed the store-remove-orphaned-snapshot-serializers branch from e02e158 to 1f29d9c Compare October 20, 2026 07:13
…emoval

kvcache-ai#4406 removed the Master-generated snapshot and restore path, but left
behind the code that only that path called. None of it has a caller
outside its own tests:

- serialize/serializer.{h,cpp}: the msgpack Serializer<T>
  specializations for allocators, buffers, replicas and mounted
  segments, SerializationHelper and SerializationError.
- ha/snapshot: StoreResourceSnapshotCodec, AllocatorSnapshotCodec and
  LocalSsdCodec.
- SegmentSerializer and SegmentView in segment.h, TaskManagerSerializer
  with the restore_task/clear_all helpers only it used, and
  LocalSsdManager::ExportPersistedState/RestorePersistedState with
  LocalSsdPersistedState.
- The detached snapshot path added by kvcache-ai#3706: SegmentPool::CaptureSnapshot
  and RestoreSnapshot, OffsetBufferAllocator and OffsetAllocator
  CaptureSnapshot/Restore with their snapshot structs and layout
  validation, AdoptMode::ReplacePool and IsKnownSegmentStatus.

SegmentPool::WriteAccess::PrepareAdopt stays in its pre-kvcache-ai#3706 form: the
read-lock test still uses it to install a blocking allocator. The
OffsetBufferAllocator node cap goes back to the literal 64Mi it had
before kvcache-ai#3706 named it OffsetAllocatorSnapshot::kMaxNodes.

DestroyedPoolAndRetainedAllocatorRemoveMetricLabels stays, rewritten to
mount directly instead of restoring a snapshot. kvcache-ai#3706 also made buffer
allocator destructors remove their segment's metric labels and made
remove_segment_metrics keep a label that still has usage; both are on
the ordinary mount path and lost their only coverage with the snapshot
tests.
@Aionw
Aionw force-pushed the store-remove-orphaned-snapshot-serializers branch from 1f29d9c to 02eb514 Compare October 20, 2026 07:23

@Icedcoco Icedcoco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI

@Aionw
Aionw merged commit 948d13c into kvcache-ai:main Oct 23, 2026
58 of 60 checks passed
0z5a added a commit to 0z5a/Mooncake that referenced this pull request Nov 1, 2026
…-sizes

Resolved conflicts:
- offset_allocator.h/.cpp: keep upstream's weak_ptr allocator handle and
  weak_from_this() call sites, adding the PR's reserved_size parameter.
- serialize/serializer.cpp: deleted upstream (kvcache-ai#4774 removed the snapshot
  serializers); accept the deletion, so the PR's restore-path changes there
  are dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants