smengcl commented on code in PR #4617:
URL: https://github.com/apache/ozone/pull/4617#discussion_r1185775695
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -666,114 +687,60 @@ private void generateSnapshotDiffReport(final String
jobKey,
final BucketLayout bucketLayout = getBucketLayout(volume, bucket,
fromSnapshot.getMetadataManager());
- final Table<String, OmKeyInfo> fsKeyTable =
- fromSnapshot.getMetadataManager().getKeyTable(bucketLayout);
- final Table<String, OmKeyInfo> tsKeyTable =
- toSnapshot.getMetadataManager().getKeyTable(bucketLayout);
-
- boolean useFullDiff = ozoneManager.getConfiguration().getBoolean(
- OZONE_OM_SNAPSHOT_FORCE_FULL_DIFF,
- OZONE_OM_SNAPSHOT_FORCE_FULL_DIFF_DEFAULT);
- if (forceFullDiff) {
- useFullDiff = true;
- }
-
Map<String, String> tablePrefixes =
getTablePrefixes(toSnapshot.getMetadataManager(), volume, bucket);
- List<String> tablesToLookUp =
- Collections.singletonList(fsKeyTable.getName());
- Set<String> deltaFilesForKeyOrFileTable = getDeltaFiles(fromSnapshot,
- toSnapshot, tablesToLookUp, fsInfo, tsInfo, useFullDiff,
- tablePrefixes, path.toString());
-
- // Workaround to handle deletes if native rocksDb tool for reading
- // tombstone is not loaded.
- // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read tombstone
- if (!isNativeRocksToolsLoaded) {
- deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
- fromSnapshot, tablesToLookUp));
- }
- try {
- addToObjectIdMap(fsKeyTable, tsKeyTable,
- Pair.of(isNativeRocksToolsLoaded, deltaFilesForKeyOrFileTable),
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- tablePrefixes);
- } catch (NativeLibraryNotLoadedException e) {
- // Workaround to handle deletes if use of native rocksDb tool for
- // reading tombstone fails.
- // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read tombstone
- deltaFilesForKeyOrFileTable.addAll(getSSTFileListForSnapshot(
- fromSnapshot, tablesToLookUp));
- try {
- addToObjectIdMap(fsKeyTable, tsKeyTable,
- Pair.of(false, deltaFilesForKeyOrFileTable),
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- tablePrefixes);
- } catch (NativeLibraryNotLoadedException ex) {
- // This code should be never executed.
- throw new IllegalStateException(ex);
- }
- }
+
+ boolean useFullDiff = snapshotForceFullDiff || forceFullDiff;
+
+ validateSnapshotsAreActive(volume, bucket, fromSnapshotName,
+ toSnapshotName);
+ Table<String, OmKeyInfo> fsKeyTable = fromSnapshot.getMetadataManager()
+ .getKeyTable(bucketLayout);
+ Table<String, OmKeyInfo> tsKeyTable = toSnapshot.getMetadataManager()
+ .getKeyTable(bucketLayout);
+
+ getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsKeyTable, tsKeyTable,
+ fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
+ tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
+ path.toString());
if (bucketLayout.isFileSystemOptimized()) {
- final Table<String, OmDirectoryInfo> fsDirTable =
+ validateSnapshotsAreActive(volume, bucket, fromSnapshotName,
+ toSnapshotName);
+
+ Table<String, OmDirectoryInfo> fsDirTable =
fromSnapshot.getMetadataManager().getDirectoryTable();
- final Table<String, OmDirectoryInfo> tsDirTable =
+ Table<String, OmDirectoryInfo> tsDirTable =
toSnapshot.getMetadataManager().getDirectoryTable();
- tablesToLookUp = Collections.singletonList(fsDirTable.getName());
- final Set<String> deltaFilesForDirTable =
- getDeltaFiles(fromSnapshot, toSnapshot, tablesToLookUp, fsInfo,
- tsInfo, useFullDiff, tablePrefixes, path.toString());
- if (!isNativeRocksToolsLoaded) {
- deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
- fromSnapshot, tablesToLookUp));
- }
- try {
- addToObjectIdMap(fsDirTable, tsDirTable,
- Pair.of(isNativeRocksToolsLoaded, deltaFilesForDirTable),
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot,
- objectIDsToCheckMap,
- tablePrefixes);
- } catch (NativeLibraryNotLoadedException e) {
- try {
- // Workaround to handle deletes if use of native rockstools for
- // reading tombstone fails.
- // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read
tombstone
- deltaFilesForDirTable.addAll(getSSTFileListForSnapshot(
- fromSnapshot, tablesToLookUp));
- addToObjectIdMap(fsDirTable, tsDirTable,
- Pair.of(false, deltaFilesForDirTable),
- objectIdToKeyNameMapForFromSnapshot,
- objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- tablePrefixes);
- } catch (NativeLibraryNotLoadedException ex) {
- // This code should be never executed.
- throw new IllegalStateException(ex);
- }
- }
+
+ getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsDirTable, tsDirTable,
+ fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
+ tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
+ objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
+ path.toString());
}
+ validateSnapshotsAreActive(volume, bucket, fromSnapshotName,
+ toSnapshotName);
long totalDiffEntries = generateDiffReport(jobId,
objectIDsToCheckMap,
objectIdToKeyNameMapForFromSnapshot,
objectIdToKeyNameMapForToSnapshot);
updateJobStatusToDone(jobKey, totalDiffEntries);
- } catch (IOException | RocksDBException exception) {
+ } catch (ExecutionException | IOException | RocksDBException exception) {
updateJobStatus(jobKey, IN_PROGRESS, FAILED);
- LOG.error("Caught checked exception during diff report generation for " +
+ LOG.info("Caught checked exception during diff report generation for " +
Review Comment:
Why is log level changed to `info` for exception message?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -360,49 +364,51 @@ private Set<String> getSSTFileListForSnapshot(OmSnapshot
snapshot,
.getPath(), tablesToLookUp);
}
- @SuppressWarnings("parameternumber")
- public SnapshotDiffResponse getSnapshotDiffReport(
- final String volume,
- final String bucket,
- final OmSnapshot fromSnapshot,
- final OmSnapshot toSnapshot,
- final SnapshotInfo fsInfo,
- final SnapshotInfo tsInfo,
- final int index,
- final int pageSize,
- final boolean forceFullDiff
+ public SnapshotDiffResponse getSnapshotDiffReport(final String volume,
+ final String bucket,
+ final String fromSnapshot,
+ final String toSnapshot,
+ final int index,
+ final int pageSize,
+ final boolean forceFullDiff
) throws IOException {
Review Comment:
Thx for refactoring the parameter list!
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -786,29 +753,88 @@ private void generateSnapshotDiffReport(final String
jobKey,
}
}
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private void getDeltaFilesAndDiffKeysToObjectIdToKeyMap(
+ final Table<String, ? extends WithObjectID> fsTable,
+ final Table<String, ? extends WithObjectID> tsTable,
+ final OmSnapshot fromSnapshot,
+ final OmSnapshot toSnapshot,
+ final SnapshotInfo fsInfo,
+ final SnapshotInfo tsInfo,
+ final boolean useFullDiff,
+ final Map<String, String> tablePrefixes,
+ final PersistentMap<byte[], byte[]> oldObjIdToKeyMap,
+ final PersistentMap<byte[], byte[]> newObjIdToKeyMap,
+ final PersistentSet<byte[]> objectIDsToCheck,
+ final String diffDir
+ ) throws IOException, RocksDBException {
+
+ List<String> tablesToLookUp = Collections.singletonList(fsTable.getName());
+
+ Set<String> deltaFiles = getDeltaFiles(fromSnapshot, toSnapshot,
+ tablesToLookUp, fsInfo, tsInfo, useFullDiff, tablePrefixes, diffDir);
+
+ // Workaround to handle deletes if native rocksDb tool for reading
+ // tombstone is not loaded.
+ // TODO: [SNAPSHOT] Update Rocksdb SSTFileIterator to read tombstone
+ if (!sstDumpTool.isPresent()) {
+ deltaFiles.addAll(getSSTFileListForSnapshot(fromSnapshot,
+ tablesToLookUp));
+ }
+
+ try {
+ addToObjectIdMap(fsTable,
+ tsTable,
+ deltaFiles,
+ sstDumpTool.isPresent(),
+ oldObjIdToKeyMap,
+ newObjIdToKeyMap,
+ objectIDsToCheck,
+ tablePrefixes);
+ } catch (NativeLibraryNotLoadedException e) {
+ try {
Review Comment:
Not related. But good to have a log message here just in case we would catch
some bugs with it.
```suggestion
} catch (NativeLibraryNotLoadedException e) {
LOG.warn("SSTDumpTool load failure, retrying without it", e);
try {
```
--
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]
