[Store] Extract master snapshot codec from MasterService（3/5）#2831
[Store] Extract master snapshot codec from MasterService（3/5）#2831xiangui33423 wants to merge 7 commits into
Conversation
xiangui33423
requested review from
00fish0,
Libotry,
XucSh,
YiXR,
stmatengss and
ykwd
as code owners
This change extracts snapshot serialization/deserialization logic into a
dedicated MasterSnapshotCodec class while maintaining 100% backward
compatibility with existing snapshot formats.
Changes:
- Add ha::MasterSnapshotCodec for encoding/decoding master snapshots
- Add MasterSnapshotStateView as a read-only view of live state
- Update MasterSnapshotManager to use the new codec
- Codec delegates to existing serializers (MetadataSerializer,
SegmentSerializer, TaskManagerSerializer) to preserve format
- Add basic unit tests for codec functionality
The codec provides a clean interface with Encode() and Decode() methods
that return payloads as a map: {"metadata", "segments", "task_manager"}.
This sets the foundation for future refactoring of individual serializers
while keeping this change small and mergeable.
Snapshot format remains byte-identical to previous implementation:
- Metadata: msgpack-encoded, zstd-compressed per-shard
- Segments: msgpack-encoded segment manager state
- Task manager: msgpack-encoded task manager state
- Manifest: messagepack|1.0.0|master
Related to snapshot refactoring issue - PR 3 of the series.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces MasterSnapshotCodec and MasterSnapshotStateView to encapsulate the serialization and deserialization of master snapshot payloads, replacing inline logic in MasterSnapshotManager and adding comprehensive unit tests. Feedback focuses on improving const-correctness and type safety: specifically, changing MasterSnapshotStateView and the codec's encoding methods to use non-const references to eliminate internal const_cast operations, and replacing the std::unordered_map payload container with a dedicated struct to avoid map lookup overhead and potential runtime exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| struct MasterSnapshotStateView { | ||
| const MasterService& master_service; | ||
| const SegmentManager& segment_manager; | ||
| const NoFSegmentManager& nof_segment_manager; | ||
| const ClientTaskManager& task_manager; | ||
|
|
||
| MasterSnapshotStateView(const MasterService& ms, | ||
| const SegmentManager& sm, | ||
| const NoFSegmentManager& nsm, | ||
| const ClientTaskManager& tm) | ||
| : master_service(ms), | ||
| segment_manager(sm), | ||
| nof_segment_manager(nsm), | ||
| task_manager(tm) {} | ||
| }; |
There was a problem hiding this comment.
The MasterSnapshotStateView struct currently holds const references to the live state components, but the underlying serializers (e.g., MetadataSerializer, SegmentSerializer, TaskManagerSerializer) require non-const pointers. This forces the codec to use const_cast internally during encoding, which violates const-correctness and is a code smell.
Since the live state components are non-const in the manager anyway, we should change MasterSnapshotStateView to hold non-const references. This allows us to completely eliminate all const_casts in the codec implementation.
| struct MasterSnapshotStateView { | |
| const MasterService& master_service; | |
| const SegmentManager& segment_manager; | |
| const NoFSegmentManager& nof_segment_manager; | |
| const ClientTaskManager& task_manager; | |
| MasterSnapshotStateView(const MasterService& ms, | |
| const SegmentManager& sm, | |
| const NoFSegmentManager& nsm, | |
| const ClientTaskManager& tm) | |
| : master_service(ms), | |
| segment_manager(sm), | |
| nof_segment_manager(nsm), | |
| task_manager(tm) {} | |
| }; | |
| struct MasterSnapshotStateView { | |
| MasterService& master_service; | |
| SegmentManager& segment_manager; | |
| NoFSegmentManager& nof_segment_manager; | |
| ClientTaskManager& task_manager; | |
| MasterSnapshotStateView(MasterService& ms, | |
| SegmentManager& sm, | |
| NoFSegmentManager& nsm, | |
| ClientTaskManager& tm) | |
| : master_service(ms), | |
| segment_manager(sm), | |
| nof_segment_manager(nsm), | |
| task_manager(tm) {} | |
| }; |
| tl::expected<std::vector<uint8_t>, SerializationError> | ||
| MasterSnapshotCodec::EncodeMetadata( | ||
| const MasterService& master_service) const { | ||
| // Delegate to the existing MetadataSerializer for now. | ||
| // This maintains the exact same format as before. | ||
| MasterService::MetadataSerializer serializer( | ||
| const_cast<MasterService*>(&master_service)); | ||
| return serializer.Serialize(); | ||
| } |
There was a problem hiding this comment.
By changing the parameter of EncodeMetadata to a non-const reference (matching the suggested non-const MasterSnapshotStateView), we can safely eliminate the const_cast here.
tl::expected<std::vector<uint8_t>, SerializationError>
MasterSnapshotCodec::EncodeMetadata(
MasterService& master_service) const {
// Delegate to the existing MetadataSerializer for now.
// This maintains the exact same format as before.
MasterService::MetadataSerializer serializer(&master_service);
return serializer.Serialize();
}| tl::expected<std::vector<uint8_t>, SerializationError> | ||
| MasterSnapshotCodec::EncodeSegments( | ||
| const SegmentManager& segment_manager, | ||
| const NoFSegmentManager& nof_segment_manager) const { | ||
| // Use the existing SegmentSerializer which only handles SegmentManager | ||
| // Note: NoFSegmentManager is not currently serialized in snapshots | ||
| SegmentSerializer serializer(const_cast<SegmentManager*>(&segment_manager)); | ||
| return serializer.Serialize(); | ||
| } |
There was a problem hiding this comment.
By changing the parameter of EncodeSegments to a non-const reference, we can safely eliminate the const_cast here.
| tl::expected<std::vector<uint8_t>, SerializationError> | |
| MasterSnapshotCodec::EncodeSegments( | |
| const SegmentManager& segment_manager, | |
| const NoFSegmentManager& nof_segment_manager) const { | |
| // Use the existing SegmentSerializer which only handles SegmentManager | |
| // Note: NoFSegmentManager is not currently serialized in snapshots | |
| SegmentSerializer serializer(const_cast<SegmentManager*>(&segment_manager)); | |
| return serializer.Serialize(); | |
| } | |
| tl::expected<std::vector<uint8_t>, SerializationError> | |
| MasterSnapshotCodec::EncodeSegments( | |
| SegmentManager& segment_manager, | |
| const NoFSegmentManager& nof_segment_manager) const { | |
| // Use the existing SegmentSerializer which only handles SegmentManager | |
| // Note: NoFSegmentManager is not currently serialized in snapshots | |
| SegmentSerializer serializer(&segment_manager); | |
| return serializer.Serialize(); | |
| } |
| tl::expected<std::vector<uint8_t>, SerializationError> | ||
| MasterSnapshotCodec::EncodeTaskManager( | ||
| const ClientTaskManager& task_manager) const { | ||
| // Use the existing TaskManagerSerializer | ||
| TaskManagerSerializer serializer( | ||
| const_cast<ClientTaskManager*>(&task_manager)); | ||
| return serializer.Serialize(); | ||
| } |
There was a problem hiding this comment.
By changing the parameter of EncodeTaskManager to a non-const reference, we can safely eliminate the const_cast here.
tl::expected<std::vector<uint8_t>, SerializationError>
MasterSnapshotCodec::EncodeTaskManager(
ClientTaskManager& task_manager) const {
// Use the existing TaskManagerSerializer
TaskManagerSerializer serializer(&task_manager);
return serializer.Serialize();
}| const auto& payloads = encode_result.value(); | ||
| const auto& serialized_metadata = payloads.at("metadata"); | ||
| const auto& serialized_segment = payloads.at("segments"); | ||
| const auto& serialized_task_manager = payloads.at("task_manager"); |
There was a problem hiding this comment.
Using std::unordered_map<std::string, std::vector<uint8_t>> to pass a fixed set of serialized payloads introduces unnecessary overhead (string hashing, map allocations, lookups) and runtime risks (e.g., std::out_of_range exceptions when using .at()).
Consider defining a dedicated structure like MasterSnapshotPayloads with explicit fields for metadata, segments, and task_manager. This completely eliminates map lookups, hashing, and potential exceptions, making the interface robust, self-documenting, and type-safe.
…y in snapshot codec
- Replace const references with non-const references in MasterSnapshotStateView
to eliminate all const_cast operations in encoding methods
- Replace std::unordered_map with dedicated MasterSnapshotPayloads struct
for type-safe, zero-overhead payload container
- Update Encode() to return MasterSnapshotPayloads instead of map
- Update Decode() to accept MasterSnapshotPayloads instead of map
- Simplify payload access from map.at("key") to struct.field
Benefits:
- Eliminates const_cast code smell and improves const-correctness
- Removes map lookup overhead and string hashing
- Provides compile-time guarantee that all payloads are present
- Eliminates potential std::out_of_range runtime exceptions
- More self-documenting and IDE-friendly interface
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
One minor issue, though it not block this PR: However, since |
|
Agreed. MetadataSerializer is still tightly coupled to several MasterService internals, including metadata shards, discarded replicas and their locking, routing indexes, the segment manager, and |
…codec test Replace MasterSnapshotCodec::GetManifestContent() with EncodeManifest(type, version, snapshot_id). The old helper hardcoded "messagepack|1.0.0|master", silently dropping the snapshot_id and changing the third manifest field from the original "type|version|snapshot_id" format. PersistState now passes the real snapshot_id, restoring the on-disk format. Serializer identifiers are exposed as codec constants (kSerializerType/kSerializerVersion). Register master_snapshot_codec_test in CMakeLists and rewrite it against the current MasterSnapshotPayloads struct API (the file was still using the removed unordered_map payload API and never compiled). Private state access goes through a befriended fixture helper. Tests cover manifest encoding, encode/decode round-trip, corrupt-payload, and null-service. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add missing '// namespace ha' closing-brace comment flagged by the clang-format CI check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| return payloads; | ||
| } | ||
|
|
||
| tl::expected<void, SerializationError> MasterSnapshotCodec::Decode( |
There was a problem hiding this comment.
Decode() should preserve the existing restore order: segments → metadata → task manager. Deserializing a MEMORY replica requires its segment/allocator to be restored first; otherwise, GetMountedSegment() returns SEGMENT_NOT_FOUND. Please also add a round-trip test containing a memory replica.
Decode() must restore segments before metadata, matching the original RestoreState() order. A MEMORY replica's allocator is bound to its mounted segment, so restoring metadata first makes GetMountedSegment() return SEGMENT_NOT_FOUND while deserializing the replica. Add a round-trip test that mounts a segment, stores a MEMORY replica, and verifies it is queryable after decode into a fresh service. The test fails with the previous metadata-first order (SEGMENT_NOT_FOUND). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ore/extract-snapshot-codec
PR4 Refactoring Completion ReportOverviewSuccessfully completed the refactoring work suggested in the code review for PR#2820, implementing layered decoupling of snapshot save and restore logic. Implemented Changes1. New Files
|
xiangui33423
force-pushed
the
store/extract-snapshot-codec
branch
from
70b0f81 to
6132c65
Compare
Extract snapshot restore logic into Repository, Codec, and Service layers for better separation of concerns and testability. Changes: - Add snapshot_constants.h for shared snapshot file/format constants - Extend MasterSnapshotRepository with three new methods: * LoadLatestSnapshot() - load latest snapshot descriptor from catalog * LoadRestoreCandidates() - load list of restorable snapshot candidates * DownloadSnapshotPayloads() - download snapshot data from object store - Add MasterService::ApplySnapshotState() for post-decode state application - Refactor RestoreState() to use three-phase architecture: Phase 1: Repository loads snapshot candidates Phase 2: Repository downloads + Codec decodes payloads Phase 3: MasterService applies decoded state and rebuilds metrics - Update all files to use shared snapshot constants This refactoring addresses code review feedback from PR#2820, improving maintainability and making each layer independently testable. Testing: master_snapshot_codec_test (5/5 passed), sample snapshot tests passed Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Description
Relates to #2688.
Part 3 of 5 in the series refactoring master snapshot handling out of the
monolithic
MasterService.This PR extracts snapshot serialization/deserialization (the codec) into a
dedicated
ha::MasterSnapshotCodecclass, following the earlier extractions:MasterSnapshotManager([Store] Refactor: Extract snapshot orchestration into MasterSnapshotManager #2805, merged)MasterSnapshotManager::Stop()non-blocking + repository split (merged)MasterServiceWhat changed
ha::MasterSnapshotCodecowns all encode/decode logic for the mastersnapshot payload (metadata shards, segment manager state, task manager state,
discarded replicas, and the
manifest.txtformat descriptor).ha::MasterSnapshotStateView, a lightweight read-only DTO holdingconstreferences to the live state components (MasterService,SegmentManager,NoFSegmentManager,ClientTaskManager). This gives thecodec a clean interface without copying large state.
MasterSnapshotManagernow delegates encode/decode to the codec instead ofinlining the serialization (
master_snapshot_manager.cpp: 62 lines slimmeddown to delegation).
MasterServicegrantsfriendaccess toha::MasterSnapshotCodecso thecodec can read private state during serialization.
The on-disk snapshot format is unchanged — the codec reproduces the existing
messagepack|1.0.0|masterformat exactly, so it remains backward compatible withsnapshots produced before this refactor.
Files
Module
mooncake-store)Type of Change
How Has This Been Tested?
Added
master_snapshot_codec_test.cppcovering the codec in isolation:GetManifestContent— manifest descriptor is correctEncodeDecodeRoundTrip— encode → decode restores equivalent stateDecodeWithMissingPayload— missing payload is handled gracefullyDecodeWithNullService— null service target is rejectedTest commands:
Test results:
Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks pass