You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2023/01/09 22:26:29 UTC
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4119: HDDS-7690. [Snapshot] Use SST file list output from compaction DAG as SnapshotDiff input
hemantk-12 commented on code in PR #4119:
URL: https://github.com/apache/ozone/pull/4119#discussion_r1065107437
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -107,38 +165,41 @@ public SnapshotDiffReport getSnapshotDiffReport(final String volume,
private void addToObjectIdMap(Table<String, ? extends WithObjectID> fsTable,
Table<String, ? extends WithObjectID> tsTable, Set<String> deltaFiles,
Map<Long, String> oldObjIdToKeyMap, Map<Long, String> newObjIdToKeyMap,
- Set<Long> objectIDsToCheck, boolean isDirectoryTable)
- throws RocksDBException {
+ Set<Long> objectIDsToCheck, boolean isDirectoryTable) {
+
if (deltaFiles.isEmpty()) {
return;
}
- final Stream<String> keysToCheck =
- new ManagedSstFileReader(deltaFiles).getKeyStream();
- keysToCheck.forEach(key -> {
- try {
- final WithObjectID oldKey = fsTable.get(key);
- final WithObjectID newKey = tsTable.get(key);
- if (areKeysEqual(oldKey, newKey)) {
- // We don't have to do anything.
- return;
- }
- if (oldKey != null) {
- final long oldObjId = oldKey.getObjectID();
- oldObjIdToKeyMap
- .put(oldObjId, getKeyOrDirectoryName(isDirectoryTable, oldKey));
- objectIDsToCheck.add(oldObjId);
+ try (Stream<String> keysToCheck = new ManagedSstFileReader(deltaFiles)
Review Comment:
nice.
##########
hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java:
##########
@@ -710,13 +710,64 @@ public synchronized void loadAllCompactionLogs() {
}
}
+ /**
+ * Helper function that prepends SST file name with SST backup directory path,
+ * and appends the extension '.sst'.
+ */
+ private String getSSTFullPathInBackupDir(String sstFilenameWithoutExtension,
Review Comment:
I don't think method name is matching what method itself. Name and description comment say that it returns the full path of SST file *in backup directory* with '.sst' extension. But that's not true, if compaction didn't happen and file doesn't exist in backup directory.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -44,24 +50,74 @@
import java.util.Set;
import java.util.stream.Stream;
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
/**
* Class to generate snapshot diff.
*/
public class SnapshotDiffManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SnapshotDiffManager.class);
+ private RocksDBCheckpointDiffer differ;
+
+ public SnapshotDiffManager(RocksDBCheckpointDiffer differ) {
+ this.differ = differ;
+ }
+
+ /**
+ * Copied straight from TestOMSnapshotDAG. TODO: Dedup. Move this to util.
+ */
+ private Map<String, String> getTablePrefixes(
+ OMMetadataManager omMetadataManager,
+ String volumeName, String bucketName) throws IOException {
+ HashMap<String, String> tablePrefixes = new HashMap<>();
Review Comment:
```suggestion
Map<String, String> tablePrefixes = new HashMap<>();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -152,19 +213,67 @@ private String getKeyOrDirectoryName(boolean isDirectory,
}
@NotNull
+ @SuppressWarnings("parameternumber")
private Set<String> getDeltaFiles(OmSnapshot fromSnapshot,
- OmSnapshot toSnapshot, List<String> tablesToLookUp)
- throws RocksDBException {
- Set<String> fromSnapshotFiles = RdbUtil.getSSTFilesForComparison(
- fromSnapshot.getMetadataManager().getStore().getDbLocation().getPath(),
- tablesToLookUp);
- Set<String> toSnapshotFiles = RdbUtil.getSSTFilesForComparison(
- toSnapshot.getMetadataManager().getStore().getDbLocation().getPath(),
- tablesToLookUp);
+ OmSnapshot toSnapshot, List<String> tablesToLookUp,
+ SnapshotInfo fsInfo, SnapshotInfo tsInfo,
+ String volume, String bucket)
+ throws RocksDBException, IOException {
+ // TODO: Refactor the parameter list
final Set<String> deltaFiles = new HashSet<>();
- deltaFiles.addAll(fromSnapshotFiles);
- deltaFiles.addAll(toSnapshotFiles);
+
+ // Check if compaction DAG is available, use that if so
+ if (differ != null && fsInfo != null && tsInfo != null) {
+ // Construct DifferSnapshotInfo
+ final DifferSnapshotInfo fromDSI =
+ getDSIFromSI(fsInfo, fromSnapshot, volume, bucket);
+ final DifferSnapshotInfo toDSI =
+ getDSIFromSI(tsInfo, toSnapshot, volume, bucket);
+
+ LOG.debug("Calling RocksDBCheckpointDiffer");
+ List<String> sstDiffList =
+ differ.getSSTDiffListWithFullPath(toDSI, fromDSI);
+ LOG.debug("SST diff list: {}", sstDiffList);
Review Comment:
nit: This is duplicate. It gets logged in RocksDBCheckpointDiffer: https://github.com/apache/ozone/blob/9fe6d106acc71fa79402880ba6344fcab131185d/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java#L750
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -44,24 +50,74 @@
import java.util.Set;
import java.util.stream.Stream;
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
/**
* Class to generate snapshot diff.
*/
public class SnapshotDiffManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SnapshotDiffManager.class);
+ private RocksDBCheckpointDiffer differ;
+
+ public SnapshotDiffManager(RocksDBCheckpointDiffer differ) {
+ this.differ = differ;
+ }
+
+ /**
+ * Copied straight from TestOMSnapshotDAG. TODO: Dedup. Move this to util.
Review Comment:
This should be just a comment not java-doc comment.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -44,24 +50,74 @@
import java.util.Set;
import java.util.stream.Stream;
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
/**
* Class to generate snapshot diff.
*/
public class SnapshotDiffManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SnapshotDiffManager.class);
+ private RocksDBCheckpointDiffer differ;
+
+ public SnapshotDiffManager(RocksDBCheckpointDiffer differ) {
+ this.differ = differ;
+ }
+
+ /**
+ * Copied straight from TestOMSnapshotDAG. TODO: Dedup. Move this to util.
+ */
+ private Map<String, String> getTablePrefixes(
+ OMMetadataManager omMetadataManager,
+ String volumeName, String bucketName) throws IOException {
+ HashMap<String, String> tablePrefixes = new HashMap<>();
+ String volumeId = String.valueOf(omMetadataManager.getVolumeId(volumeName));
+ String bucketId = String.valueOf(
+ omMetadataManager.getBucketId(volumeName, bucketName));
+ tablePrefixes.put(OmMetadataManagerImpl.KEY_TABLE,
+ OM_KEY_PREFIX + volumeName + OM_KEY_PREFIX + bucketName);
+ tablePrefixes.put(OmMetadataManagerImpl.FILE_TABLE,
+ OM_KEY_PREFIX + volumeId + OM_KEY_PREFIX + bucketId);
+ tablePrefixes.put(OmMetadataManagerImpl.DIRECTORY_TABLE,
+ OM_KEY_PREFIX + volumeId + OM_KEY_PREFIX + bucketId);
+ return tablePrefixes;
+ }
+
+ /**
+ * 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();
+ final String snapshotId = snapshotInfo.getSnapshotID();
+ final long dbTxSequenceNumber = snapshotInfo.getDbTxSequenceNumber();
+
+ DifferSnapshotInfo dsi = new DifferSnapshotInfo(
Review Comment:
```suggestion
return new DifferSnapshotInfo(
checkpointPath,
snapshotId,
dbTxSequenceNumber,
getTablePrefixes(snapshotOMMM, volumeName, bucketName));
```
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org