Copilot commented on code in PR #9268:
URL: https://github.com/apache/ozone/pull/9268#discussion_r2524763961
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -863,53 +816,109 @@ public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnap
* must be non-null.
* @return A list of SST files without extension. e.g. ["000050", "000060"]
*/
- public synchronized Optional<List<String>> getSSTDiffList(DifferSnapshotInfo
src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup) {
+ public synchronized Optional<List<SstFileInfo>>
getSSTDiffList(DifferSnapshotVersion src,
+ DifferSnapshotVersion dest, TablePrefixInfo prefixInfo, Set<String>
tablesToLookup, boolean useCompactionDag) {
// TODO: Reject or swap if dest is taken after src, once snapshot chain
// integration is done.
- Set<String> srcSnapFiles = readRocksDBLiveFiles(src.getRocksDB(),
tablesToLookup);
- Set<String> destSnapFiles = readRocksDBLiveFiles(dest.getRocksDB(),
tablesToLookup);
-
- Set<String> fwdDAGSameFiles = new HashSet<>();
- Set<String> fwdDAGDifferentFiles = new HashSet<>();
-
- LOG.debug("Doing forward diff from src '{}' to dest '{}'",
- src.getDbPath(), dest.getDbPath());
- internalGetSSTDiffList(src, dest, srcSnapFiles, destSnapFiles,
- fwdDAGSameFiles, fwdDAGDifferentFiles);
+ Map<String, SstFileInfo> fwdDAGSameFiles = new HashMap<>();
+ Map<String, SstFileInfo> fwdDAGDifferentFiles = new HashMap<>();
+ if (useCompactionDag) {
+ LOG.debug("Doing forward diff from src '{}' to dest '{}'",
src.getDbPath(), dest.getDbPath());
+ internalGetSSTDiffList(src, dest, fwdDAGSameFiles, fwdDAGDifferentFiles);
+ } else {
+ Set<SstFileInfo> srcSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
+ Set<SstFileInfo> destSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
Review Comment:
The method `getSSTDiffList` is using `src.getSstFileMap().values()` for both
`srcSstFileInfos` and `destSstFileInfos` on line 832. This appears to be a bug
as it should be using `dest.getSstFileMap().values()` for the destination
snapshot.
This will cause the diff computation to incorrectly compare the source
snapshot against itself, resulting in inaccurate delta files.
```suggestion
Set<SstFileInfo> destSstFileInfos = new
HashSet<>(dest.getSstFileMap().values());
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/DifferSnapshotInfo.java:
##########
@@ -17,56 +17,68 @@
package org.apache.ozone.rocksdiff;
+import static java.util.function.Function.identity;
+import static java.util.stream.Collectors.toMap;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.NavigableMap;
+import java.util.Set;
import java.util.UUID;
-import org.apache.hadoop.hdds.utils.db.TablePrefixInfo;
-import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.apache.ozone.rocksdb.util.SstFileInfo;
/**
* Snapshot information node class for the differ.
*/
public class DifferSnapshotInfo {
- private final String dbPath;
- private final UUID snapshotId;
- private final long snapshotGeneration;
+ private final UUID id;
Review Comment:
[nitpick] The variable name `id` on line 37 in `DifferSnapshotInfo` is too
generic. Since this represents a snapshot identifier, it should be renamed to
`snapshotId` to match the original naming and improve code clarity.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -863,53 +816,109 @@ public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnap
* must be non-null.
* @return A list of SST files without extension. e.g. ["000050", "000060"]
*/
- public synchronized Optional<List<String>> getSSTDiffList(DifferSnapshotInfo
src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup) {
+ public synchronized Optional<List<SstFileInfo>>
getSSTDiffList(DifferSnapshotVersion src,
+ DifferSnapshotVersion dest, TablePrefixInfo prefixInfo, Set<String>
tablesToLookup, boolean useCompactionDag) {
// TODO: Reject or swap if dest is taken after src, once snapshot chain
// integration is done.
- Set<String> srcSnapFiles = readRocksDBLiveFiles(src.getRocksDB(),
tablesToLookup);
- Set<String> destSnapFiles = readRocksDBLiveFiles(dest.getRocksDB(),
tablesToLookup);
-
- Set<String> fwdDAGSameFiles = new HashSet<>();
- Set<String> fwdDAGDifferentFiles = new HashSet<>();
-
- LOG.debug("Doing forward diff from src '{}' to dest '{}'",
- src.getDbPath(), dest.getDbPath());
- internalGetSSTDiffList(src, dest, srcSnapFiles, destSnapFiles,
- fwdDAGSameFiles, fwdDAGDifferentFiles);
+ Map<String, SstFileInfo> fwdDAGSameFiles = new HashMap<>();
+ Map<String, SstFileInfo> fwdDAGDifferentFiles = new HashMap<>();
+ if (useCompactionDag) {
+ LOG.debug("Doing forward diff from src '{}' to dest '{}'",
src.getDbPath(), dest.getDbPath());
+ internalGetSSTDiffList(src, dest, fwdDAGSameFiles, fwdDAGDifferentFiles);
+ } else {
+ Set<SstFileInfo> srcSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
+ Set<SstFileInfo> destSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
+ for (SstFileInfo srcSstFileInfo : srcSstFileInfos) {
+ if (destSstFileInfos.contains(srcSstFileInfo)) {
+ fwdDAGSameFiles.put(srcSstFileInfo.getFileName(), srcSstFileInfo);
+ } else {
+ fwdDAGDifferentFiles.put(srcSstFileInfo.getFileName(),
srcSstFileInfo);
+ }
+ }
+ for (SstFileInfo destSstFileInfo : destSstFileInfos) {
+ if (srcSstFileInfos.contains(destSstFileInfo)) {
+ fwdDAGSameFiles.put(destSstFileInfo.getFileName(), destSstFileInfo);
+ } else {
+ fwdDAGDifferentFiles.put(destSstFileInfo.getFileName(),
destSstFileInfo);
+ }
+ }
+ }
if (LOG.isDebugEnabled()) {
LOG.debug("Result of diff from src '" + src.getDbPath() + "' to dest '" +
dest.getDbPath() + "':");
StringBuilder logSB = new StringBuilder();
logSB.append("Fwd DAG same SST files: ");
- for (String file : fwdDAGSameFiles) {
+ for (String file : fwdDAGSameFiles.keySet()) {
logSB.append(file).append(SPACE_DELIMITER);
}
LOG.debug(logSB.toString());
logSB.setLength(0);
logSB.append("Fwd DAG different SST files: ");
- for (String file : fwdDAGDifferentFiles) {
+ for (String file : fwdDAGDifferentFiles.keySet()) {
logSB.append(file).append(SPACE_DELIMITER);
}
LOG.debug("{}", logSB);
}
// Check if the DAG traversal was able to reach all the destination SST
files.
- for (String destSnapFile : destSnapFiles) {
- if (!fwdDAGSameFiles.contains(destSnapFile) &&
!fwdDAGDifferentFiles.contains(destSnapFile)) {
+ for (String destSnapFile : dest.getSstFiles()) {
+ if (!fwdDAGSameFiles.containsKey(destSnapFile) &&
!fwdDAGDifferentFiles.containsKey(destSnapFile)) {
return Optional.empty();
}
}
- if (src.getTablePrefixes() != null && src.getTablePrefixes().size() != 0) {
- RocksDiffUtils.filterRelevantSstFiles(fwdDAGDifferentFiles,
src.getTablePrefixes(),
- compactionDag.getCompactionMap(), tablesToLookup, src.getRocksDB(),
dest.getRocksDB());
+ if (prefixInfo != null && prefixInfo.size() != 0) {
+ RocksDiffUtils.filterRelevantSstFiles(fwdDAGDifferentFiles,
tablesToLookup, prefixInfo);
+ }
+ return Optional.of(new ArrayList<>(fwdDAGDifferentFiles.values()));
+ }
+
+ /**
+ * This class represents a version of a snapshot in a database differ
operation.
+ * It contains metadata associated with a specific snapshot version,
including
+ * SST file information, generation id, and the database path for the given
version.
+ *
+ * Designed to work with `DifferSnapshotInfo`, this class allows the
retrieval of
+ * snapshot-related metadata and facilitates mapping of SST files for
version comparison
+ * and other operations.
+ *
+ * The core functionality is to store and provide read-only access to:
+ * - SST file information for a specified snapshot version.
+ * - Snapshot generation identifier.
+ * - Path to the database directory corresponding to the snapshot version.
+ */
+ public static class DifferSnapshotVersion {
+ private Map<String, SstFileInfo> sstFiles;
+ private long generation;
+ private Path dbPath;
+
+ public DifferSnapshotVersion(DifferSnapshotInfo differSnapshotInfo, int
version,
+ Set<String> tablesToLookup) {
+ this.sstFiles = differSnapshotInfo.getSstFiles(version, tablesToLookup)
+ .stream().collect(Collectors.toMap(SstFileInfo::getFileName,
identity()));
+ this.generation = differSnapshotInfo.getGeneration();
+ this.dbPath = differSnapshotInfo.getDbPath(version);
+ }
+
+ private Path getDbPath() {
+ return dbPath;
+ }
+
+ private long getGeneration() {
+ return generation;
+ }
+
+ private Set<String> getSstFiles() {
Review Comment:
[nitpick] The variable name `getSstFiles` returns a `Set<String>` but reads
more like it should return file objects. Consider renaming to `getSstFileNames`
for better clarity about what is being returned.
```suggestion
private Set<String> getSstFileNames() {
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -832,22 +778,29 @@ private String getSSTFullPath(String
sstFilenameWithoutExtension,
* "/path/to/sstBackupDir/000060.sst"]
*/
public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnapshotInfo src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup,
- String sstFilesDirForSnapDiffJob) {
+ DifferSnapshotInfo dest, Map<Integer, Integer> versionMap,
TablePrefixInfo prefixInfo,
+ Set<String> tablesToLookup, String sstFilesDirForSnapDiffJob) throws
IOException {
+ int srcVersion = src.getMaxVersion();
+ if (!versionMap.containsKey(srcVersion)) {
+ throw new IOException("No corresponding dest version corresponding
srcVersion : " + srcVersion + " in " +
+ "versionMap : " + versionMap);
Review Comment:
Typo in error message: "corresponding to" should be used instead of
"corresponding srcVersion". The message should read: "No corresponding dest
version for srcVersion: " + srcVersion + " in versionMap: " + versionMap
```suggestion
throw new IOException("No corresponding dest version for srcVersion: "
+ srcVersion + " in versionMap: " + versionMap);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1061,10 +1069,12 @@ private void getDeltaFilesAndDiffKeysToObjectIdToKeyMap(
// tombstone is not loaded.
// TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read tombstone
if (skipNativeDiff || !isNativeLibsLoaded) {
- Set<String> inputFiles = getSSTFileListForSnapshot(fromSnapshot,
tablesToLookUp);
- ManagedRocksDB fromDB =
((RDBStore)fromSnapshot.getMetadataManager().getStore()).getDb().getManagedRocksDb();
- RocksDiffUtils.filterRelevantSstFiles(inputFiles, tablePrefixes,
tablesToLookUp, fromDB);
- deltaFiles.addAll(inputFiles);
+ Set<SstFileInfo> inputFiles =
filterRelevantSstFiles(getSSTFileListForSnapshot(fromSnapshot, tablesToLookUp),
+ tablesToLookUp, tablePrefixes);
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbPath();
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -863,53 +816,109 @@ public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnap
* must be non-null.
* @return A list of SST files without extension. e.g. ["000050", "000060"]
*/
- public synchronized Optional<List<String>> getSSTDiffList(DifferSnapshotInfo
src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup) {
+ public synchronized Optional<List<SstFileInfo>>
getSSTDiffList(DifferSnapshotVersion src,
+ DifferSnapshotVersion dest, TablePrefixInfo prefixInfo, Set<String>
tablesToLookup, boolean useCompactionDag) {
// TODO: Reject or swap if dest is taken after src, once snapshot chain
// integration is done.
- Set<String> srcSnapFiles = readRocksDBLiveFiles(src.getRocksDB(),
tablesToLookup);
- Set<String> destSnapFiles = readRocksDBLiveFiles(dest.getRocksDB(),
tablesToLookup);
-
- Set<String> fwdDAGSameFiles = new HashSet<>();
- Set<String> fwdDAGDifferentFiles = new HashSet<>();
-
- LOG.debug("Doing forward diff from src '{}' to dest '{}'",
- src.getDbPath(), dest.getDbPath());
- internalGetSSTDiffList(src, dest, srcSnapFiles, destSnapFiles,
- fwdDAGSameFiles, fwdDAGDifferentFiles);
+ Map<String, SstFileInfo> fwdDAGSameFiles = new HashMap<>();
+ Map<String, SstFileInfo> fwdDAGDifferentFiles = new HashMap<>();
+ if (useCompactionDag) {
+ LOG.debug("Doing forward diff from src '{}' to dest '{}'",
src.getDbPath(), dest.getDbPath());
+ internalGetSSTDiffList(src, dest, fwdDAGSameFiles, fwdDAGDifferentFiles);
+ } else {
+ Set<SstFileInfo> srcSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
+ Set<SstFileInfo> destSstFileInfos = new
HashSet<>(src.getSstFileMap().values());
+ for (SstFileInfo srcSstFileInfo : srcSstFileInfos) {
+ if (destSstFileInfos.contains(srcSstFileInfo)) {
+ fwdDAGSameFiles.put(srcSstFileInfo.getFileName(), srcSstFileInfo);
+ } else {
+ fwdDAGDifferentFiles.put(srcSstFileInfo.getFileName(),
srcSstFileInfo);
+ }
+ }
+ for (SstFileInfo destSstFileInfo : destSstFileInfos) {
+ if (srcSstFileInfos.contains(destSstFileInfo)) {
+ fwdDAGSameFiles.put(destSstFileInfo.getFileName(), destSstFileInfo);
+ } else {
+ fwdDAGDifferentFiles.put(destSstFileInfo.getFileName(),
destSstFileInfo);
+ }
+ }
+ }
if (LOG.isDebugEnabled()) {
LOG.debug("Result of diff from src '" + src.getDbPath() + "' to dest '" +
dest.getDbPath() + "':");
StringBuilder logSB = new StringBuilder();
logSB.append("Fwd DAG same SST files: ");
- for (String file : fwdDAGSameFiles) {
+ for (String file : fwdDAGSameFiles.keySet()) {
logSB.append(file).append(SPACE_DELIMITER);
}
LOG.debug(logSB.toString());
logSB.setLength(0);
logSB.append("Fwd DAG different SST files: ");
- for (String file : fwdDAGDifferentFiles) {
+ for (String file : fwdDAGDifferentFiles.keySet()) {
logSB.append(file).append(SPACE_DELIMITER);
}
LOG.debug("{}", logSB);
}
// Check if the DAG traversal was able to reach all the destination SST
files.
- for (String destSnapFile : destSnapFiles) {
- if (!fwdDAGSameFiles.contains(destSnapFile) &&
!fwdDAGDifferentFiles.contains(destSnapFile)) {
+ for (String destSnapFile : dest.getSstFiles()) {
+ if (!fwdDAGSameFiles.containsKey(destSnapFile) &&
!fwdDAGDifferentFiles.containsKey(destSnapFile)) {
return Optional.empty();
}
}
- if (src.getTablePrefixes() != null && src.getTablePrefixes().size() != 0) {
- RocksDiffUtils.filterRelevantSstFiles(fwdDAGDifferentFiles,
src.getTablePrefixes(),
- compactionDag.getCompactionMap(), tablesToLookup, src.getRocksDB(),
dest.getRocksDB());
+ if (prefixInfo != null && prefixInfo.size() != 0) {
+ RocksDiffUtils.filterRelevantSstFiles(fwdDAGDifferentFiles,
tablesToLookup, prefixInfo);
+ }
+ return Optional.of(new ArrayList<>(fwdDAGDifferentFiles.values()));
+ }
+
+ /**
+ * This class represents a version of a snapshot in a database differ
operation.
+ * It contains metadata associated with a specific snapshot version,
including
+ * SST file information, generation id, and the database path for the given
version.
+ *
+ * Designed to work with `DifferSnapshotInfo`, this class allows the
retrieval of
+ * snapshot-related metadata and facilitates mapping of SST files for
version comparison
+ * and other operations.
+ *
+ * The core functionality is to store and provide read-only access to:
+ * - SST file information for a specified snapshot version.
+ * - Snapshot generation identifier.
+ * - Path to the database directory corresponding to the snapshot version.
+ */
+ public static class DifferSnapshotVersion {
+ private Map<String, SstFileInfo> sstFiles;
+ private long generation;
+ private Path dbPath;
+
+ public DifferSnapshotVersion(DifferSnapshotInfo differSnapshotInfo, int
version,
+ Set<String> tablesToLookup) {
+ this.sstFiles = differSnapshotInfo.getSstFiles(version, tablesToLookup)
+ .stream().collect(Collectors.toMap(SstFileInfo::getFileName,
identity()));
+ this.generation = differSnapshotInfo.getGeneration();
+ this.dbPath = differSnapshotInfo.getDbPath(version);
+ }
+
+ private Path getDbPath() {
+ return dbPath;
+ }
+
+ private long getGeneration() {
+ return generation;
+ }
+
+ private Set<String> getSstFiles() {
+ return sstFiles.keySet();
+ }
+
+ private Map<String, SstFileInfo> getSstFileMap() {
Review Comment:
[nitpick] The newly added `DifferSnapshotVersion` class has private getter
methods (`getDbPath()`, `getGeneration()`, `getSstFiles()`, `getSstFileMap()`)
on lines 907-921. These private methods are only accessed within the
`RocksDBCheckpointDiffer` class. Consider whether these should be
package-private or if the encapsulation is appropriate for this inner/nested
class design.
```suggestion
Path getDbPath() {
return dbPath;
}
long getGeneration() {
return generation;
}
Set<String> getSstFiles() {
return sstFiles.keySet();
}
Map<String, SstFileInfo> getSstFileMap() {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1215,25 +1221,42 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
- private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp) {
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp,
+ TablePrefixInfo tablePrefixInfo) {
Set<String> diffFiles;
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
+ Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbPath().toAbsolutePath();
Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDbPath().toAbsolutePath();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1215,25 +1221,42 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
- private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp) {
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp,
+ TablePrefixInfo tablePrefixInfo) {
Set<String> diffFiles;
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
+ Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDatabasePath();
Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDatabasePath();
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -832,22 +778,29 @@ private String getSSTFullPath(String
sstFilenameWithoutExtension,
* "/path/to/sstBackupDir/000060.sst"]
*/
public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnapshotInfo src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup,
- String sstFilesDirForSnapDiffJob) {
+ DifferSnapshotInfo dest, Map<Integer, Integer> versionMap,
TablePrefixInfo prefixInfo,
+ Set<String> tablesToLookup, String sstFilesDirForSnapDiffJob) throws
IOException {
+ int srcVersion = src.getMaxVersion();
Review Comment:
The method `getMaxVersion()` on line 75 returns `Integer` which can be null
if the `versionSstFiles` map is empty. However, the method is called on line
783 without null checking, which could result in a `NullPointerException`.
Although line 373-375 in `SnapshotDiffManager.java` includes an empty check,
it's better to make `getMaxVersion()` return `int` and throw an exception
internally if the map is empty, or document that it may return null and handle
it at all call sites.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -350,37 +361,34 @@ private void deleteDir(Path path) {
/**
* Convert from SnapshotInfo to DifferSnapshotInfo.
*/
- private DifferSnapshotInfo getDSIFromSI(SnapshotInfo snapshotInfo,
- OmSnapshot omSnapshot, final String volumeName, final String bucketName)
- throws IOException {
-
- final OMMetadataManager snapshotOMMM = omSnapshot.getMetadataManager();
- final String checkpointPath =
- snapshotOMMM.getStore().getDbLocation().getPath();
+ private static DifferSnapshotInfo getDSIFromSI(OMMetadataManager
activeOmMetadataManager,
+ SnapshotInfo snapshotInfo, OmSnapshotLocalData snapshotLocalData) throws
IOException {
Review Comment:
[nitpick] The method `getDSIFromSI` has been changed from an instance method
to a static method, but the method name remains obscure. Consider renaming this
to something more descriptive like `createDifferSnapshotInfo` or
`convertToDifferSnapshotInfo` to improve code readability.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1215,25 +1221,42 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
- private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp) {
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp,
+ TablePrefixInfo tablePrefixInfo) {
Set<String> diffFiles;
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
+ Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
Path fromSnapshotPath = ((RDBStore)
fromSnapshot.getMetadataManager().getStore()).getDbPath();
Path toSnapshotPath = ((RDBStore)
toSnapshot.getMetadataManager().getStore()).getDbPath();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1215,25 +1221,42 @@ Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
toSnapshot.getSnapshotTableKey()));
}
- private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp) {
+ private Set<String> getDiffFiles(OmSnapshot fromSnapshot, OmSnapshot
toSnapshot, Set<String> tablesToLookUp,
+ TablePrefixInfo tablePrefixInfo) {
Set<String> diffFiles;
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
+ Path toSnapshotPath =
toSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
Path fromSnapshotPath =
Paths.get(fromSnapshot.getMetadataManager().getStore().getDbPath());
Path toSnapshotPath =
Paths.get(toSnapshot.getMetadataManager().getStore().getDbPath());
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -832,22 +778,29 @@ private String getSSTFullPath(String
sstFilenameWithoutExtension,
* "/path/to/sstBackupDir/000060.sst"]
*/
public synchronized Optional<List<String>>
getSSTDiffListWithFullPath(DifferSnapshotInfo src,
- DifferSnapshotInfo dest, Set<String> tablesToLookup,
- String sstFilesDirForSnapDiffJob) {
+ DifferSnapshotInfo dest, Map<Integer, Integer> versionMap,
TablePrefixInfo prefixInfo,
+ Set<String> tablesToLookup, String sstFilesDirForSnapDiffJob) throws
IOException {
+ int srcVersion = src.getMaxVersion();
+ if (!versionMap.containsKey(srcVersion)) {
+ throw new IOException("No corresponding dest version corresponding
srcVersion : " + srcVersion + " in " +
+ "versionMap : " + versionMap);
Review Comment:
There's an inconsistency in exception handling. When `versionMap` doesn't
contain the expected version on lines 784-787, an `IOException` is thrown.
However, the exception message could be more helpful by including the available
versions in the map to aid debugging.
Consider updating the error message to: `"No corresponding dest version for
srcVersion: " + srcVersion + ". Available versions in versionMap: " +
versionMap.keySet()`
```suggestion
throw new IOException("No corresponding dest version for srcVersion: "
+ srcVersion +
". Available versions in versionMap: " + versionMap.keySet());
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1061,10 +1069,12 @@ private void getDeltaFilesAndDiffKeysToObjectIdToKeyMap(
// tombstone is not loaded.
// TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read tombstone
if (skipNativeDiff || !isNativeLibsLoaded) {
- Set<String> inputFiles = getSSTFileListForSnapshot(fromSnapshot,
tablesToLookUp);
- ManagedRocksDB fromDB =
((RDBStore)fromSnapshot.getMetadataManager().getStore()).getDb().getManagedRocksDb();
- RocksDiffUtils.filterRelevantSstFiles(inputFiles, tablePrefixes,
tablesToLookUp, fromDB);
- deltaFiles.addAll(inputFiles);
+ Set<SstFileInfo> inputFiles =
filterRelevantSstFiles(getSSTFileListForSnapshot(fromSnapshot, tablesToLookUp),
+ tablesToLookUp, tablePrefixes);
+ Path fromSnapshotPath =
fromSnapshot.getMetadataManager().getStore().getDbLocation().getAbsoluteFile().toPath();
Review Comment:
Access of [element](1) annotated with VisibleForTesting found in production
code.
```suggestion
// Avoid using getDbLocation() annotated with @VisibleForTesting.
// Use a public API to get the DB path, or refactor to pass it in.
Path fromSnapshotPath = Paths.get(fromSnapshot.getSnapshotPath());
```
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/DifferSnapshotInfo.java:
##########
@@ -17,56 +17,68 @@
package org.apache.ozone.rocksdiff;
+import static java.util.function.Function.identity;
+import static java.util.stream.Collectors.toMap;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.NavigableMap;
+import java.util.Set;
import java.util.UUID;
-import org.apache.hadoop.hdds.utils.db.TablePrefixInfo;
-import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.apache.ozone.rocksdb.util.SstFileInfo;
/**
* Snapshot information node class for the differ.
*/
public class DifferSnapshotInfo {
- private final String dbPath;
- private final UUID snapshotId;
- private final long snapshotGeneration;
+ private final UUID id;
+ private final long generation;
+ private final Function<Integer, Path> dbPathFunction;
+ private final NavigableMap<Integer, List<SstFileInfo>> versionSstFiles;
- private final TablePrefixInfo tablePrefixes;
+ public DifferSnapshotInfo(Function<Integer, Path> dbPathFunction, UUID id,
long gen,
+ NavigableMap<Integer, List<SstFileInfo>> sstFiles)
{
+ this.dbPathFunction = dbPathFunction;
+ this.id = id;
+ generation = gen;
+ this.versionSstFiles = sstFiles;
+ }
- private final ManagedRocksDB rocksDB;
+ public Path getDbPath(int version) {
+ return dbPathFunction.apply(version);
+ }
- public DifferSnapshotInfo(String db, UUID id, long gen,
- TablePrefixInfo tablePrefixInfo,
- ManagedRocksDB rocksDB) {
- dbPath = db;
- snapshotId = id;
- snapshotGeneration = gen;
- tablePrefixes = tablePrefixInfo;
- this.rocksDB = rocksDB;
+ public UUID getId() {
+ return id;
}
- public String getDbPath() {
- return dbPath;
+ public long getGeneration() {
+ return generation;
}
- public UUID getSnapshotId() {
- return snapshotId;
+ List<SstFileInfo> getSstFiles(int version, Set<String> tablesToLookup) {
+ return versionSstFiles.get(version).stream()
+ .filter(sstFileInfo ->
tablesToLookup.contains(sstFileInfo.getColumnFamily()))
+ .collect(Collectors.toList());
}
- public long getSnapshotGeneration() {
- return snapshotGeneration;
+ @VisibleForTesting
+ SstFileInfo getSstFile(int version, String fileName) {
+ return versionSstFiles.get(version).stream()
+ .filter(sstFileInfo -> sstFileInfo.getFileName().equals(fileName))
+ .findFirst().orElse(null);
}
- public TablePrefixInfo getTablePrefixes() {
- return tablePrefixes;
+ Integer getMaxVersion() {
+ return versionSstFiles.lastKey();
}
@Override
public String toString() {
- return String.format("DifferSnapshotInfo{dbPath='%s', snapshotID='%s', " +
- "snapshotGeneration=%d, tablePrefixes size=%s}",
- dbPath, snapshotId, snapshotGeneration, tablePrefixes.size());
- }
-
- public ManagedRocksDB getRocksDB() {
- return rocksDB;
+ return String.format("DifferSnapshotInfo{dbPath='%s', id='%s',
generation=%d}",
+ versionSstFiles.keySet().stream().collect(toMap(identity(),
dbPathFunction::apply)), id, generation);
Review Comment:
The `toString()` method on lines 80-83 constructs a map using
`collect(toMap(identity(), dbPathFunction::apply))` which could fail with
`IllegalStateException` if there are duplicate keys. While unlikely in
practice, a safer implementation would use a merge function or better yet,
format the output differently to avoid this potential issue.
```suggestion
String dbPaths = versionSstFiles.keySet().stream()
.map(version -> version + "=" + dbPathFunction.apply(version))
.collect(Collectors.joining(", ", "{", "}"));
return String.format("DifferSnapshotInfo{dbPath='%s', id='%s',
generation=%d}", dbPaths, id, generation);
```
--
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]
