jojochuang commented on PR #10778:
URL: https://github.com/apache/ozone/pull/10778#issuecomment-5064178365
## Potential issue: same-object RENAME/MODIFY are not ordered
`SnapDiffDependencyGraph` indexes multiple non-delete nodes per `objectId`
(e.g. a `RENAME` + a `MODIFY` for the same object), but it never adds an edge
**between** those entries. Topological sort can therefore emit the `RENAME`
before a `MODIFY` that is reported at the from-snapshot (pre-rename) source
path — and once the path has been renamed away, that `MODIFY` can no longer be
applied at its reported path.
### Concrete example
Object `2` under dir `1`, from `parent/a.txt` (v1) to `parent/b.txt` (v2) —
renamed **and** modified. Snapdiff emits, with `MODIFY` at the from-snapshot
path:
```
RENAME source="parent/a.txt" target="parent/b.txt" (objectId=2, parent=1)
MODIFY source="parent/a.txt" (objectId=2, parent=1)
```
No edge links the two nodes, so both have `inDegree == 0` and Kahn's sort
emits them in input order: `[RENAME, MODIFY]`. A consumer then renames
`parent/a.txt -> parent/b.txt` and tries to modify the now-nonexistent
`parent/a.txt`. Correct order is `[MODIFY, RENAME]`.
Note the existing `testModifyAndRenameForSameObjectKeepDependencyOrder` does
not catch this: it only asserts the parent `CREATE` is first and that the two
tail entries share `objectId == 2` — it never asserts the RENAME-vs-MODIFY
relative order, and its data uses `MODIFY` at the rename *target* path.
### Reproduction test (fails on current code)
```java
@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);
}
```
```
expected: <[MODIFY, RENAME]> but was: <[RENAME, MODIFY]>
```
### Suggested fix
Add 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 `TestSnapDiffDependencyGraph` cases pass (11/11).
I pushed a candidate fix + the reproduction test to a branch for reference:
https://github.com/jojochuang/ozone/tree/HDDS-15390-rename-modify-order-fix
_Found via automated review (Cursor Bugbot)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
