smengcl commented on code in PR #10750:
URL: https://github.com/apache/ozone/pull/10750#discussion_r3709630858
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1354,6 +1356,104 @@ private String resolveBucketRelativePath(boolean
isFSOBucket,
.substring(1);
}
+ /**
+ * Returns whether any ancestor of the given parent is a deleted directory.
+ * The walk stops at renamed directories because their descendant deletes are
+ * not subsumed by a deleted ancestor above them.
+ *
+ * @param objectIdToParentId from-snapshot directory parent graph keyed by
stable
+ * objectId; must not be the to-snapshot graph
+ * @param renamedDirectoryIds objectIds of directories renamed in the diff
(from
+ * snapshot objectIds, not destination paths)
+ */
+ @VisibleForTesting
+ boolean hasDeletedAncestor(long parentObjectId, Set<Long>
deletedDirectoryIds,
+ Set<Long> renamedDirectoryIds, Map<Long, Long> objectIdToParentId,
+ long bucketObjectId, Map<Long, Boolean> ancestorMemo) {
+ Objects.requireNonNull(objectIdToParentId, "objectIdToParentId must not be
null");
+ Objects.requireNonNull(renamedDirectoryIds, "renamedDirectoryIds must not
be null");
+
+ if (parentObjectId == bucketObjectId) {
+ return false;
+ }
+ Boolean cached = ancestorMemo.get(parentObjectId);
+ if (cached != null) {
+ return cached;
+ }
+
+ List<Long> path = new ArrayList<>();
+ long current = parentObjectId;
+ boolean result;
+ while (true) {
+ if (current == bucketObjectId) {
+ result = false;
+ break;
+ }
+ cached = ancestorMemo.get(current);
+ if (cached != null) {
+ result = cached;
+ break;
+ }
+ if (renamedDirectoryIds.contains(current)) {
+ result = false;
+ ancestorMemo.put(current, false);
+ break;
+ }
+ if (deletedDirectoryIds.contains(current)) {
+ result = true;
+ ancestorMemo.put(current, true);
+ break;
+ }
+ Long nextParent = objectIdToParentId.get(current);
+ if (nextParent == null) {
+ result = false;
+ ancestorMemo.put(current, false);
+ break;
+ }
Review Comment:
Should this condition fail the job instead?
The method contract requires a complete parent graph from the source
snapshot. The bucket case is handled above. Therefore, a missing non-bucket
parent means that the graph is incomplete.
If it returns false. The caller then keeps the entry as a top-level delete.
The report can contain a redundant child delete.
```diff
Long nextParent = objectIdToParentId.get(current);
if (nextParent == null) {
- result = false;
- ancestorMemo.put(current, false);
- break;
+ throw new IllegalStateException(
+ "Missing parent for object ID: " + current);
}
```
Pls add test case for this as well.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1354,6 +1356,104 @@ private String resolveBucketRelativePath(boolean
isFSOBucket,
.substring(1);
}
+ /**
+ * Returns whether any ancestor of the given parent is a deleted directory.
+ * The walk stops at renamed directories because their descendant deletes are
+ * not subsumed by a deleted ancestor above them.
+ *
+ * @param objectIdToParentId from-snapshot directory parent graph keyed by
stable
+ * objectId; must not be the to-snapshot graph
+ * @param renamedDirectoryIds objectIds of directories renamed in the diff
(from
+ * snapshot objectIds, not destination paths)
+ */
+ @VisibleForTesting
+ boolean hasDeletedAncestor(long parentObjectId, Set<Long>
deletedDirectoryIds,
+ Set<Long> renamedDirectoryIds, Map<Long, Long> objectIdToParentId,
+ long bucketObjectId, Map<Long, Boolean> ancestorMemo) {
+ Objects.requireNonNull(objectIdToParentId, "objectIdToParentId must not be
null");
+ Objects.requireNonNull(renamedDirectoryIds, "renamedDirectoryIds must not
be null");
+
+ if (parentObjectId == bucketObjectId) {
+ return false;
+ }
+ Boolean cached = ancestorMemo.get(parentObjectId);
+ if (cached != null) {
+ return cached;
+ }
+
+ List<Long> path = new ArrayList<>();
+ long current = parentObjectId;
+ boolean result;
+ while (true) {
+ if (current == bucketObjectId) {
+ result = false;
+ break;
+ }
+ cached = ancestorMemo.get(current);
+ if (cached != null) {
+ result = cached;
+ break;
+ }
+ if (renamedDirectoryIds.contains(current)) {
+ result = false;
+ ancestorMemo.put(current, false);
+ break;
+ }
+ if (deletedDirectoryIds.contains(current)) {
+ result = true;
+ ancestorMemo.put(current, true);
+ break;
+ }
+ Long nextParent = objectIdToParentId.get(current);
+ if (nextParent == null) {
+ result = false;
+ ancestorMemo.put(current, false);
+ break;
+ }
+ path.add(current);
+ current = nextParent;
+ }
+ for (Long node : path) {
+ ancestorMemo.put(node, result);
+ }
+ return result;
+ }
+
+ /**
+ * Filters mixed directory and file delete entries, retaining only those
without
+ * a deleted directory ancestor. FSO buckets only.
+ *
+ * @param objectIdToParentId from-snapshot directory parent graph keyed by
stable
+ * objectId, built from a full fromSnapshot directoryTable scan
+ * @param renamedDirectoryIds objectIds of directories renamed in the diff
(from
+ * snapshot objectIds); pass an empty set when there are no renames
+ */
+ @VisibleForTesting
+ <T extends WithParentObjectId> List<T> filterTopLevelDeletedEntries(
Review Comment:
Could this method use the existing persistent snapshot-diff structures?
This receives all delete entries and the full parent graph. It also creates
a HashSet, a HashMap, and an ArrayList for the job. These objects use OM heap
memory for the complete pass.
The configured snapshot-diff limit is 1,000,000,000 changed keys. A large
FSO delete can exhaust the OM heap.
Pls read the entries from persistent storage and write the retained entries
to persistent storage. If this is not possible, pls enforce a safe limit before
this method runs. I am fine with this being in a follow-up task.
--
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]
