HDDS-15390. Utility to generate dependency ordered snapdiff report. - #10778
HDDS-15390. Utility to generate dependency ordered snapdiff report.#10778SaketaChalamchala wants to merge 11 commits into
Conversation
…encyGraph The dependency graph indexed multiple non-delete nodes per objectId (e.g. RENAME + MODIFY) but never added an edge between them, so topological sort could emit RENAME before a MODIFY reported at the from-snapshot source path. Add an intra-object edge oriented by which path the entry carries, plus a reproduction test. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: Ib7ebd867f58b864f17ec28f61843390a34cf0705
Potential issue: same-object RENAME/MODIFY are not ordered
Concrete exampleObject No edge links the two nodes, so both have Note the existing Reproduction test (fails on current code)@Test
void testModifyAtSourcePathOrderedBeforeRename() {
List<SnapDiffDependencyEntry> entries = Arrays.asList(
entry(2L, 1L, RENAME, "parent/a.txt", "parent/b.txt"),
entry(2L, 1L, MODIFY, "parent/a.txt"));
List<DiffType> orderedTypes = toDiffTypes(sort(entries));
assertEquals(Arrays.asList(MODIFY, RENAME), orderedTypes);
}Suggested fixAdd an intra-object edge oriented by which path the entry carries: an entry at the RENAME source path must precede the RENAME; the RENAME must precede an entry at its target path. With that edge, the reproduction test and all existing I pushed a candidate fix + the reproduction test to a branch for reference: Found via automated review (Cursor Bugbot). |
smengcl
left a comment
There was a problem hiding this comment.
Thanks @SaketaChalamchala for the patch.
smengcl
left a comment
There was a problem hiding this comment.
Thanks @SaketaChalamchala for the update
| int[] deleteReady = new int[nodeCount]; | ||
| int[] otherReady = new int[nodeCount]; |
There was a problem hiding this comment.
Thanks for replacing the maps and sets with primitive arrays. That reduces memory usage.
However, the graph still keeps all diff entries in memory. It also creates several arrays with one element for each entry. This method creates three more arrays with nodeCount elements.
With a limit of one billion changed keys, these arrays require tens of GB:
Assuming 64-bit HotSpot with compressed 4-byte references:
- Sorting retains five
intarrays and two reference arrays with aboutNelements:(5 × 4N) + (2 × 4N) = 28Nbytes. IfN = 1B, this is 28 GB (26.1 GiB).adjTargetsadds4 × Ebytes. IfE = N = 1B, this adds 4 GB (3.7 GiB).- Each path-index mapping needs approximately a 32-byte
HashMap.Node, a 16-byte boxedInteger, and at least 4 bytes of bucket storage: at least 52 bytes.- One billion mappings therefore need at least 52 GB (48.4 GiB).
- Rename-heavy input creates two mappings per entry. If
N = 1B, thenM = 2N, which needs at least 104 GB (96.9 GiB).Estimates exclude the entries, report objects, and decoded path strings.
Shall we have a follow-up task for bounded or batched processing?
There was a problem hiding this comment.
Yes, the memory footprint can become large for large diffs. The 1B changed keys limit is currently being checked against the total estimated keys in the delta file set to decide whether the snapshot diff can be accepted/rejected.
I propose we add another guardrail limiting the number of actual diff entries to 1M in order to be eligible for in-memory dependency ordering enforced in HDDS-15391. (This limit may also be useful in prior stages of the diff to decide whether to spill to disk).
As a follow-up task enable RocksDB backed graph for larger diffs and fallback to current order meanwhile.
What do you think?
More memory optimizations in the latest commit:
- Free cached path strings after edge building.
- Presize the four path-index HashMaps to their exact per-category counts.
- Presize the edges buffer to max(INITIAL_EDGE_CAPACITY, nodeCount) so realistic graphs (edge density 3–8 per node) skip most doubling copies.
- Rework buildObjectIdGroups to track only objectIds that have a RENAME (the only objectIds that can pick up intra-object edges).
There was a problem hiding this comment.
Thanks @SaketaChalamchala .
AFAICT the current producer still reports a directory MODIFY from newParentIdPathMap, but reports a key MODIFY at oldKey. The Javadoc does not match this behavior.
RENAME A -> B, RENAME X -> A, and directory MODIFY A/child still cause a false cycle.
Pls identify the path side for each MODIFY entry, and test both source-side key and target-side directory cases.
There was a problem hiding this comment.
I extended the skip toSnapshot path dependencies to re-occupied paths and added more tests to cover these scenarios.
Currently there is no producer for this dependency graph. The plan is implement the merge join diff HDDS-15391 separate from the baseline snapdiff and have it be the producer to the dependency graph. The Merge Join diff will always outptut MODIFY entries from the fromSnapshot namespace regardless of whether it is a file/key/directory.
There was a problem hiding this comment.
Pull request overview
Adds utilities for generating replay-safe, dependency-ordered snapshot diff reports.
Changes:
- Adds dependency metadata wrappers for diff entries.
- Builds and topologically sorts dependency graphs.
- Adds unit coverage for hierarchy, path conflicts, rename chains, and cycles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
SnapDiffDependencyEntry.java |
Wraps diff entries with object and parent metadata. |
SnapDiffDependencyGraph.java |
Builds dependency edges and orders entries. |
TestSnapDiffDependencyGraph.java |
Tests dependency ordering and cycle detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static String getToSnapshotPath(SnapDiffDependencyEntry entry) { | ||
| if (entry.getDiffType() == DiffType.RENAME) { | ||
| return entry.getTargetPath(); | ||
| } | ||
| return entry.getSourcePath(); |
There was a problem hiding this comment.
A directory MODIFY can use the to-snapshot path, so excluding all MODIFY entries from this pass is not sufficient. The entry must identify which snapshot contains its path. Pls add tests for a source-side modified key and a target-side modified directory.
There was a problem hiding this comment.
Updated the patch to skip to-snapshot dependency edges for ancestors that have been recreated (deleted and created again) and added a test to verify that the dependencies are resolved without creating a cycle.
@smengcl directory MODIFY using the to-snapshot path and file/key MODIFY using from-snapshot path seems like an inconsistency in reporting. Both report MODIFY path but it is not intuitive that file/key report path from from-snapshot and directory from to-snapshot.
When reporting a diff between from-snapshot and to-snapshot I think it makes sense to report "File/Key/Dir X in fromSnapshot was modified" rather than "File/Key/Dir X in toSnapshot was modified prior to taking the snapshot". Added a javadoc in SnapDiffDependencyEntry that expects the same.
This was what was intended to come out of HDDS-15391 as well.
…aths to avoid cycles.
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 snapshot diff reports order entries by diff type (
DELETEs first, thenRENAME/CREATE/MODIFY). That ordering is not always safe to replay. For example, ifA/Bis renamed toC/Band directoryAis deleted, replaying theDELETEbefore theRENAMEfails on because the rename source no longer exists.This PR adds isolated utilities for dependency-ordered snapshot diff report generation.
SnapDiffDependencyEntry
SnapDiffDependencyGraph
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15390
How was this patch tested?
Unit Test.