swamirishi commented on code in PR #8423:
URL: https://github.com/apache/ozone/pull/8423#discussion_r2084307465
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
@@ -49,4 +54,19 @@ public static Set<String> getSSTFilesForComparison(
.map(lfm -> new File(lfm.path(), lfm.fileName()).getPath())
.collect(Collectors.toCollection(HashSet::new));
}
+
+ public static Map<Object, String> getSSTFilesWithInodesForComparison(final
ManagedRocksDB rocksDB, List<String> cfs) {
+ return getLiveSSTFilesForCFs(rocksDB, cfs).stream()
+ .collect(Collectors.toMap(
+ lfm -> {
+ try {
+ return Files.readAttributes(new File(lfm.path(),
lfm.fileName()).toPath(),
Review Comment:
Use OmSnapshotUtils.getINode() function instead
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1217,6 +1223,23 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, List<String> tablesToLookUp) {
+ Set<String> diffFiles;
+ try {
+ Map<Object, String> fromSnapshotFiles =
getSSTFileMapForSnapshot(fromSnapshot, tablesToLookUp);
+ Map<Object, String> toSnapshotFiles =
getSSTFileMapForSnapshot(toSnapshot, tablesToLookUp);
+ diffFiles = Stream.concat(fromSnapshotFiles.entrySet().stream(),
+ toSnapshotFiles.entrySet().stream().filter(e ->
!fromSnapshotFiles.containsKey(e.getKey())))
Review Comment:
```suggestion
toSnapshotFiles.entrySet().stream().filter(e ->
!(fromSnapshotFiles.containsKey(e.getKey()) &&
toSnapshot.containsKey(e.getKey()))))
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdb/util/RdbUtil.java:
##########
@@ -49,4 +54,19 @@ public static Set<String> getSSTFilesForComparison(
.map(lfm -> new File(lfm.path(), lfm.fileName()).getPath())
.collect(Collectors.toCollection(HashSet::new));
}
+
+ public static Map<Object, String> getSSTFilesWithInodesForComparison(final
ManagedRocksDB rocksDB, List<String> cfs) {
+ return getLiveSSTFilesForCFs(rocksDB, cfs).stream()
+ .collect(Collectors.toMap(
+ lfm -> {
+ try {
+ return Files.readAttributes(new File(lfm.path(),
lfm.fileName()).toPath(),
Review Comment:
nit: Use map function in stream to directly convert liveMetadata file to
convert to full path instead of doing redundant computation of path separately
for key and value.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java:
##########
@@ -1545,20 +1545,28 @@ public void testGetDeltaFilesWithFullDiff() throws
IOException {
Mockito.doAnswer(invocation -> {
OmSnapshot snapshot = invocation.getArgument(0);
if (snapshot == fromSnapshot) {
- return Sets.newHashSet("1", "2", "3");
+ Map<Integer, String> inodeToFileMap = new HashMap<>();
+ inodeToFileMap.put(1, "1.sst");
+ inodeToFileMap.put(2, "2.sst");
+ inodeToFileMap.put(3, "3.sst");
+ return inodeToFileMap;
}
if (snapshot == toSnapshot) {
- return Sets.newHashSet("3", "4", "5");
+ Map<Integer, String> inodeToFileMap = new HashMap<>();
+ inodeToFileMap.put(1, "10.sst");
+ inodeToFileMap.put(2, "20.sst");
+ inodeToFileMap.put(4, "4.sst");
+ return inodeToFileMap;
}
- return Sets.newHashSet("6", "7", "8");
- }).when(spy).getSSTFileListForSnapshot(Mockito.any(OmSnapshot.class),
+ return null;
+ }).when(spy).getSSTFileMapForSnapshot(Mockito.any(OmSnapshot.class),
Mockito.anyList());
doNothing().when(spy).recordActivity(any(), any());
doNothing().when(spy).updateProgress(anyString(), anyDouble());
String diffJobKey = snap1 + DELIMITER + snap2;
Set<String> deltaFiles = spy.getDeltaFiles(fromSnapshot, toSnapshot,
Collections.emptyList(), snapshotInfo,
snapshotInfo, true, Collections.emptyMap(), null, diffJobKey);
- Assertions.assertEquals(Sets.newHashSet("1", "2", "3", "4", "5"),
deltaFiles);
+ Assertions.assertEquals(Sets.newHashSet("1.sst", "2.sst", "3.sst",
"4.sst"), deltaFiles);
Review Comment:
Shouldn't delta be equal to 3.sst & 4.sst ?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1217,6 +1223,23 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, List<String> tablesToLookUp) {
+ Set<String> diffFiles;
+ try {
+ Map<Object, String> fromSnapshotFiles =
getSSTFileMapForSnapshot(fromSnapshot, tablesToLookUp);
+ Map<Object, String> toSnapshotFiles =
getSSTFileMapForSnapshot(toSnapshot, tablesToLookUp);
+ diffFiles = Stream.concat(fromSnapshotFiles.entrySet().stream(),
+ toSnapshotFiles.entrySet().stream().filter(e ->
!fromSnapshotFiles.containsKey(e.getKey())))
Review Comment:
This logic seems to be wrong. You would only get diff files which are
present in toSnapshot
Consider
FromSnapshot has => 1.sst,2.sst, 3.sst
toSnapshot has => 2.sst, 3.sst, 5.sst
Diff should be 1.sst & 5.sst. But this logic will return only 5.sst
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
