Copilot commented on code in PR #11558:
URL: https://github.com/apache/ozone/pull/11558#discussion_r4290797784
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -954,6 +950,7 @@ private DirectoryList getSubDirectory(long dirObjID, String
prefix, OMMetadataMa
Iterator<Map.Entry<CacheKey<String>, CacheValue<OmDirectoryInfo>>>
cacheIter = dirTable.cacheIterator();
HashSet<String> deletedDirSet = new HashSet();
+ HashSet<String> dirSet = new HashSet();
Review Comment:
This introduces a raw-type `HashSet` instantiation which will produce
unchecked/rawtype warnings. Use the diamond operator to keep generics
consistent (e.g., `new HashSet<>()`).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -2001,6 +2005,48 @@ public void addSubDir(String key, OmDirectoryInfo dir,
String dirPath) {
subDirCount++;
}
+ /** Removes the entry at {@code index} from all parallel lists. */
+ void removeAt(int index) {
+ subDirList.remove(index);
+ subDirKeyList.remove(index);
+ subDirKeyPathList.remove(index);
+ subDirCount--;
+ }
+
+ /** Removes the entry if shouldRemoveIndex returns true. */
+ void removeIf(IntPredicate shouldRemoveIndex) {
+ for (int i = subDirCount - 1; i >= 0; i--) {
+ if (shouldRemoveIndex.test(i)) {
+ removeAt(i);
+ }
+ }
+ }
Review Comment:
Using an index-based predicate forces callers to reach back into
`getSubDirList()/getSubDirKeyList()` to evaluate removal conditions, which
couples external logic to internal list structure and indexing. A more
maintainable API would accept a predicate over the actual entry data (eg,
`(OmDirectoryInfo dir, String key, String path) -> boolean` via a small
internal functional interface), allowing filtering without exposing index-based
access patterns.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyLifecycleService.java:
##########
@@ -975,14 +973,20 @@ private DirectoryList getSubDirectory(long dirObjID,
String prefix, OMMetadataMa
numDirIterated++;
Table.KeyValue<String, OmDirectoryInfo> entry = iterator.next();
OmDirectoryInfo dir = entry.getValue();
- if (deletedDirSet.contains(entry.getKey())) {
+ if (deletedDirSet.contains(entry.getKey()) ||
dirSet.contains(entry.getKey())) {
continue;
}
if (dir.getParentObjectID() == dirObjID) {
subDirList.addSubDir(entry.getKey(), dir, dir.getName());
}
}
}
+ // Cache children (hash order) and table children (ascending) are
appended in that order; sort
+ // by name so the DFS stack pops siblings in strictly descending name
order, which canSkipDir
+ // relies on to decide a resumed directory was already scanned.
+ if (!dirSet.isEmpty()) {
+ subDirList.sortByName();
+ }
Review Comment:
The new behavioral contract here (dedupe cache-vs-table + conditional
sorting when cache contributes children) is not directly exercised by the added
tests, which only validate `DirectoryList` in isolation. Consider adding a unit
test that sets up directory table + cache containing overlapping child keys and
verifies: (1) duplicates are skipped, and (2) the final returned
`DirectoryList` is name-sorted when cache contributes children.
--
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]
