HDDS-15394. Sequential Reader of from and to snapshots for full snapshot diff. - #11083
HDDS-15394. Sequential Reader of from and to snapshots for full snapshot diff.#11083SaketaChalamchala wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for an optimized full snapshot diff implementation in OM by introducing a stage-1 sequential reader that scans snapshot RocksDB tables in order and writes compact, per-job intermediate structures (new/old lists, candidate IDs, and optional FSO directory edges). It also adds configuration to bound in-memory candidate retention and extends test utilities to support raw byte[] table iteration.
Changes:
- Added
FullDiffSequentialReaderplus compactEntryValueencoding to support sequential scan–based stage-1 full diff processing (with optional HA updateID gating). - Added
SnapDiffJobStoreto manage per-job temporary RocksDB column families and controlled in-memory vs spilled diff-candidate tracking. - Added/updated unit tests and supporting test utilities, plus a new OM config key and default XML property for in-memory candidate limits.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/diff/TestSnapshotDiffValueParser.java | Fixes package to match location under snapshot.diff. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/diff/TestFullDiffSequentialReader.java | Adds unit tests for sequential scan stage-1 behavior, gating, spilling, and FSO edge population. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapshotDiffValueParser.java | Fixes package to match snapshot.diff namespace. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/SnapDiffJobStore.java | Introduces per-job temporary CF store with batched writes and candidate spill-to-RocksDB behavior. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/FullDiffSequentialReader.java | Implements sequential scans over to/from tables and writes stage-1 intermediates with optional HA gating. |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/diff/EntryValue.java | Adds compact fixed-layout byte encoding for stage intermediates. |
| hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java | Adds config key + default for max in-memory entries per diff job before spill. |
| hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/InMemoryTestTable.java | Adds raw byte[] table factory with unsigned ordering and implements iterator(prefix, type) for tests. |
| hadoop-hdds/common/src/main/resources/ozone-default.xml | Documents default value and description for the new in-memory entry limit property. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jojochuang
left a comment
There was a problem hiding this comment.
Thanks for putting together the Stage 1 scaffolding for the optimized full snapshot diff path. The overall shape looks good: sequential raw-table scans, HA updateID gating, per-job intermediate column families, and compact EntryValue records align well with the efficient-snapdiff design doc.
This is a reasonable foundation PR — the abstractions are clear and the unit tests cover the main classification behaviors. A few items below are worth addressing before or shortly after merge.
PR description / hygiene
The Jira link is present, but the template line is still there:
(Please replace this section with the link to the Apache JIRA)
Please remove that placeholder.
Also, per ASF generative-tooling guidance, consider adding an explicit disclosure line, e.g. Generated-by: Cursor (...) instead of only "Developed with the help of Cursor AI."
Config key not wired
ozone.om.snapshot.diff.max.in.memory.entries.per.job is added to OMConfigKeys and ozone-default.xml, but SnapDiffJobStore.open(...) always uses OZONE_OM_SNAPSHOT_DIFF_MAX_IN_MEMORY_ENTRIES_PER_JOB_DEFAULT.
Other snapshot-diff limits are read from OzoneConfiguration in SnapshotDiffManager. Either wire this key now, or add a short TODO in SnapDiffJobStore.open so the integration PR doesn't miss it.
keyPrefix is untested
scanFileTables / scanDirectoryTables accept a bucket keyPrefix, but all tests scan with null. A small test with a non-null prefix would help validate the InMemoryTestTable iterator behavior before integration with real bucket-scoped scans.
From-side oldList writes all rows (confirm intent)
The design doc says the from-side directory scan "only processes entries in DiffCandidateSet," but this implementation writes every from-side row to oldList — candidates get a full signature, non-candidates get metadata with an empty signature.
That seems intentional and necessary for delete detection (e.g. object 4 in testDeleteCandidateHasMetadataWithoutSignature). Could you add a brief comment in processFromSideEntry clarifying that all from-side rows are persisted for merge-join/delete detection, not only gated candidates?
Naming: FullDiffSequentialReader vs FullDiffComputer
There is already a FullDiffComputer under om.snapshot.diff.delta (SST delta file computation). Consider a one-line class javadoc cross-reference to distinguish this Stage 1 sequential table scanner from the existing delta/SST path.
Double protobuf scan on candidates (CPU / hot path)
For to-side candidates, the hot path does two full passes over the same protobuf value:
ParsedRequiredInfo info = parseRequired(value, ...); // pass 1: walk all tags
...
byte[] signature = computeSignature(value, ...); // pass 2: walk all tags againparseKeyInfoRequiredFields extracts objectId / parentId / name / updateID and skips other fields, but still visits every tag. computeKeyInfoCompareSignature then restarts from the beginning and walks the message again for digest inputs.
HA gating avoids pass 2 for unchanged rows (present marker), which is good. But for every candidate row we still pay ~2× protobuf traversal. On large buckets that can be a substantial fraction of rows.
Suggestion: Consider a single-pass parser that returns both ParsedRequiredInfo and the compare signature in one loop, or defer signature computation until merge-join when both sides are present. Same applies to from-side candidates in processFromSideEntry.
Prefer reducing copies with ByteString on the parse path (GC)
The implementation is mostly byte[], which matches RocksDB's raw-table API (Table<byte[], byte[]>). That's appropriate at the storage boundary. However, several spots add avoidable allocations on a millions-of-rows scan:
SnapshotDiffValueParser: Already usesByteStringinternally in places, but paths likeinput.readBytes().toByteArray()copy nested protobuf fields unnecessarily. PreferByteStringinputs/overloads andCodedInputStream.newInstance(ByteString)through the parse pipeline.FullDiffSequentialReader.nameBytes():getBytes(UTF_8)allocates per FSO edge row. ConsiderByteString.copyFromUtf8(name)and materializebyte[]only atbatchPut.SnapDiffJobStore.objectIdKey()/edgeKey():ByteBuffer.allocate(...).array()allocates per key. A fixed stack buffer or reusable encoder would be cheaper.EntryValue: Store signature asByteString; exposebyte[]only intoBytes()at write time.
Guidance: Keep byte[] at the RocksDB JNI boundary; use ByteString (or zero-copy views) inside the parse/compare pipeline to avoid intermediate copies.
Minor nits
testDiffCandidatesSpillToRocksDb: Consider asserting that spill actually occurred (e.g. viaareDiffCandidatesSpilled()).InMemoryTestTable: Good addition for raw scans; please confirm unit tests pass with the newiterator()implementation.EntryValue: Consider a small dedicated round-trip test (empty name, empty signature).FullDiffSequentialReader.scanFromTable: A brief comment on why the pre-scanflushWrites()is required would help future readers.
Overall this is solid foundation work for HDDS-9154. Happy to approve; the performance items (single-pass parsing, reduced byte copying) could land in this PR or an immediate follow-up before SnapshotDiffManager integration.
| public byte[] getSignature() { | ||
| return Arrays.copyOf(signature, signature.length); | ||
| } |
There was a problem hiding this comment.
nore sure how this is going to be used, but a getter method should not be a O(n) operation. Either return signature object as is, or rename the method to copySignature().
What changes were proposed in this pull request?
Developed with the help of Cursor AI.
This PR is intended as the foundation for a more efficient snapshot diff (HDDS-9154).
Baseline full diff performs random reads against snapshot DBs and holds large in-memory maps. The optimized design replaces that with:
Table<byte[], byte[]>) soSnapshotDiffValueParseroperates on exact persisted protobuf bytes (no decode/re-encode round trip). Scan order: file/key tables first, then directory tables (FSO). Each table pair runs to-side, then from-side.updateID > fromSnapshotDbTxSequenceNumber. Rows with missing, zero, orDEFAULT_OM_UPDATE_ID (-1)are always candidates (conservative fallback).{jobId}-new-list,{jobId}-old-list— keyed by objectId{jobId}-to-edges,{jobId}-from-edges— FSO directory edges (parentId,objectId) →name{jobId}-cand-ids— spill target when the in-memory diff-candidate set exceeds the configured limitparentId,name,isDir,signature) (shared with the future DAG diff sequential reader).ozone.om.snapshot.diff.max.in.memory.entries.per.job(default 1M) bounds the in-memory diff-candidate set before spill to RocksDB.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15394
How was this patch tested?
Unit Tests.