Skip to content

HDDS-15394. Sequential Reader of from and to snapshots for full snapshot diff. - #11083

Draft
SaketaChalamchala wants to merge 2 commits into
apache:masterfrom
SaketaChalamchala:HDDS-15394
Draft

HDDS-15394. Sequential Reader of from and to snapshots for full snapshot diff.#11083
SaketaChalamchala wants to merge 2 commits into
apache:masterfrom
SaketaChalamchala:HDDS-15394

Conversation

@SaketaChalamchala

@SaketaChalamchala SaketaChalamchala commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

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:

  1. Sequential table scans over both snapshots
  2. Persisted per-job intermediate lists keyed by objectId
  3. Update-id gating (OM HA) to limit compare-signature work to likely-changed objects
  4. Deferred path resolution via FSO edge column families
  • FullDiffSequentialReader scans raw snapshot tables (Table<byte[], byte[]>) so SnapshotDiffValueParser operates 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.
  • HA gating: admits rows with updateID > fromSnapshotDbTxSequenceNumber. Rows with missing, zero, or DEFAULT_OM_UPDATE_ID (-1) are always candidates (conservative fallback).
  • SnapDiffJobStore owns per-job temporary RocksDB column families:
    • {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 limit
  • EntryValue — compact fixed-layout (parentId, name, isDir, signature) (shared with the future DAG diff sequential reader).
  • Config: 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.

@SaketaChalamchala SaketaChalamchala added snapshot https://issues.apache.org/jira/browse/HDDS-6517 AI-gen labels Aug 21, 2026
@jojochuang
jojochuang requested a lite review from Copilot August 25, 2026 16:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 FullDiffSequentialReader plus compact EntryValue encoding to support sequential scan–based stage-1 full diff processing (with optional HA updateID gating).
  • Added SnapDiffJobStore to 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 jojochuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 again

parseKeyInfoRequiredFields 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:

  1. SnapshotDiffValueParser: Already uses ByteString internally in places, but paths like input.readBytes().toByteArray() copy nested protobuf fields unnecessarily. Prefer ByteString inputs/overloads and CodedInputStream.newInstance(ByteString) through the parse pipeline.
  2. FullDiffSequentialReader.nameBytes(): getBytes(UTF_8) allocates per FSO edge row. Consider ByteString.copyFromUtf8(name) and materialize byte[] only at batchPut.
  3. SnapDiffJobStore.objectIdKey() / edgeKey(): ByteBuffer.allocate(...).array() allocates per key. A fixed stack buffer or reusable encoder would be cheaper.
  4. EntryValue: Store signature as ByteString; expose byte[] only in toBytes() 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. via areDiffCandidatesSpilled()).
  • InMemoryTestTable: Good addition for raw scans; please confirm unit tests pass with the new iterator() implementation.
  • EntryValue: Consider a small dedicated round-trip test (empty name, empty signature).
  • FullDiffSequentialReader.scanFromTable: A brief comment on why the pre-scan flushWrites() 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.

Comment on lines +77 to +79
public byte[] getSignature() {
return Arrays.copyOf(signature, signature.length);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-gen snapshot https://issues.apache.org/jira/browse/HDDS-6517

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants