Skip to content

HDDS-8939. [Snapshot] isBlockLocationSame check should be skipped if object is not OmKeyInfo. #4991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
prashantpogde merged 3 commits into from
Jun 28, 2023
Merged

HDDS-8939. [Snapshot] isBlockLocationSame check should be skipped if object is not OmKeyInfo. #4991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fa6e6f
HDDS-8939: isBlockLocationSame check should be skipped if object is n…
swamirishi Jun 27, 2023
b75e8bd
HDDS-8939: Address review comments
swamirishi Jun 27, 2023
0263cf0
HDDS-8939: Fix checkstyle
swamirishi Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import org.jetbrains.annotations.NotNull;
import org.junit.Assert;
import org.junit.AfterClass;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
import org.junit.Ignore;
Expand Down Expand Up @@ -570,6 +571,29 @@ public void testBucketDeleteIfSnapshotExists() throws Exception {
volume.deleteBucket(bucket2);
}

@Test
public void testSnapDiffWithDirRename() throws Exception {
Assume.assumeTrue(bucketLayout.isFileSystemOptimized());
String volume = "vol-" + counter.incrementAndGet();
String bucket = "buck-" + counter.incrementAndGet();
store.createVolume(volume);
OzoneVolume volume1 = store.getVolume(volume);
volume1.createBucket(bucket);
OzoneBucket bucket1 = volume1.getBucket(bucket);
bucket1.createDirectory("dir1");
String snap1 = "snap1";
createSnapshot(volume, bucket, snap1);
bucket1.renameKey("dir1", "dir1_rename");
String snap2 = "snap2";
createSnapshot(volume, bucket, snap2);
SnapshotDiffReportOzone diff = getSnapDiffReport(volume, bucket,
snap1, snap2);
Assertions.assertEquals(Arrays.asList(
SnapshotDiffReportOzone.getDiffReportEntry(
SnapshotDiffReport.DiffType.RENAME, "/dir1", "/dir1_rename")),
diff.getDiffList());
}

@Test
public void testSnapDiff() throws Exception {
String volume = "vol-" + counter.incrementAndGet();
Expand Down
67 changes: 52 additions & 15 deletions .../ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,15 +890,21 @@ void generateSnapshotDiffReport(final String jobKey,
.getKeyTable(bucketLayout);
Table<String, OmKeyInfo> tsKeyTable = toSnapshot.getMetadataManager()
.getKeyTable(bucketLayout);
Table<String, OmDirectoryInfo> fsDirTable;
Table<String, OmDirectoryInfo> tsDirTable;

final Optional<Set<Long>> oldParentIds;
final Optional<Set<Long>> newParentIds;
if (bucketLayout.isFileSystemOptimized()) {
oldParentIds = Optional.of(new HashSet<>());
newParentIds = Optional.of(new HashSet<>());
fsDirTable = fromSnapshot.getMetadataManager().getDirectoryTable();
tsDirTable = toSnapshot.getMetadataManager().getDirectoryTable();
} else {
oldParentIds = Optional.empty();
newParentIds = Optional.empty();
fsDirTable = null;
tsDirTable = null;
}

final Optional<Map<Long, Path>> oldParentIdPathMap;
Expand All @@ -924,11 +930,6 @@ void generateSnapshotDiffReport(final String jobKey,
},
() -> {
if (bucketLayout.isFileSystemOptimized()) {
Table<String, OmDirectoryInfo> fsDirTable =
fromSnapshot.getMetadataManager().getDirectoryTable();
Table<String, OmDirectoryInfo> tsDirTable =
toSnapshot.getMetadataManager().getDirectoryTable();

getDeltaFilesAndDiffKeysToObjectIdToKeyMap(fsDirTable, tsDirTable,
fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
Expand Down Expand Up @@ -959,6 +960,8 @@ void generateSnapshotDiffReport(final String jobKey,
long totalDiffEntries = generateDiffReport(jobId,
fsKeyTable,
tsKeyTable,
fsDirTable,
tsDirTable,
objectIdToDiffObject,
objectIdToKeyNameMapForFromSnapshot,
objectIdToKeyNameMapForToSnapshot,
Expand Down Expand Up @@ -1116,7 +1119,7 @@ void addToObjectIdMap(Table<String, ? extends WithParentObjectId> fsTable,
oldObjIdToKeyMap.put(rawObjId, rawValue);
SnapshotDiffObject diffObject =
createDiffObjectWithOldName(fromObjectId.getObjectID(), key,
objectIdToDiffObject.get(rawObjId));
objectIdToDiffObject.get(rawObjId), isDirectoryTable);
objectIdToDiffObject.put(rawObjId, diffObject);
oldParentIds.ifPresent(set -> set.add(
fromObjectId.getParentObjectID()));
Expand All @@ -1128,7 +1131,7 @@ void addToObjectIdMap(Table<String, ? extends WithParentObjectId> fsTable,
newObjIdToKeyMap.put(rawObjId, rawValue);
SnapshotDiffObject diffObject =
createDiffObjectWithNewName(toObjectId.getObjectID(), key,
objectIdToDiffObject.get(rawObjId));
objectIdToDiffObject.get(rawObjId), isDirectoryTable);
objectIdToDiffObject.put(rawObjId, diffObject);
newParentIds.ifPresent(set -> set.add(toObjectId
.getParentObjectID()));
Expand All @@ -1145,20 +1148,22 @@ void addToObjectIdMap(Table<String, ? extends WithParentObjectId> fsTable,
}

private SnapshotDiffObject createDiffObjectWithOldName(
long objectId, String oldName, SnapshotDiffObject diffObject) {
long objectId, String oldName, SnapshotDiffObject diffObject,
boolean isDirectory) {
SnapshotDiffObjectBuilder builder = new SnapshotDiffObjectBuilder(objectId)
.withOldKeyName(oldName);
.withOldKeyName(oldName).setIsDirectory(isDirectory);
if (diffObject != null) {
builder.withNewKeyName(diffObject.getNewKeyName());
}
return builder.build();
}

private SnapshotDiffObject createDiffObjectWithNewName(
long objectId, String newName, SnapshotDiffObject diffObject) {
long objectId, String newName, SnapshotDiffObject diffObject,
boolean isDirectory) {

SnapshotDiffObjectBuilder builder = new SnapshotDiffObjectBuilder(objectId)
.withNewKeyName(newName);
.withNewKeyName(newName).setIsDirectory(isDirectory);
if (diffObject != null) {
builder.withOldKeyName(diffObject.getOldKeyName());
}
Expand Down Expand Up @@ -1269,8 +1274,10 @@ private String resolveAbsolutePath(boolean isFSOBucket,
@SuppressWarnings({"checkstyle:ParameterNumber", "checkstyle:MethodLength"})
long generateDiffReport(
final String jobId,
final Table<String, ? extends WithObjectID> fsTable,
final Table<String, ? extends WithObjectID> tsTable,
final Table<String, OmKeyInfo> fsTable,
final Table<String, OmKeyInfo> tsTable,
final Table<String, OmDirectoryInfo> fsDirTable,
final Table<String, OmDirectoryInfo> tsDirTable,
final PersistentMap<byte[], SnapshotDiffObject> objectIdToDiffObject,
final PersistentMap<byte[], byte[]> oldObjIdToKeyMap,
final PersistentMap<byte[], byte[]> newObjIdToKeyMap,
Expand Down Expand Up @@ -1374,8 +1381,10 @@ long generateDiffReport(

// Check if block location is same or not. If it is not same,
// key must have been overridden as well.
if (!isBlockLocationSame(snapshotDiffObject.getOldKeyName(),
snapshotDiffObject.getNewKeyName(), fsTable, tsTable)) {
if (isObjectModified(snapshotDiffObject.getOldKeyName(),
snapshotDiffObject.getNewKeyName(),
snapshotDiffObject.isDirectory() ? fsDirTable : fsTable,
snapshotDiffObject.isDirectory() ? tsDirTable : tsTable)) {
// Here, oldKey name is returned as modified. Modified key name is
// based on base snapshot (from snapshot).
renameDiffs.add(codecRegistry.asRawData(
Expand Down Expand Up @@ -1439,6 +1448,34 @@ long generateDiffReport(
}
}

private boolean isObjectModified(String fromObjectName, String toObjectName,
final Table<String, ? extends WithObjectID> fromSnapshotTable,
final Table<String, ? extends WithObjectID> toSnapshotTable)
throws IOException {
Objects.requireNonNull(fromObjectName, "fromObjectName is null.");
Objects.requireNonNull(toObjectName, "toObjectName is null.");

final WithObjectID fromObject = fromSnapshotTable.get(fromObjectName);
final WithObjectID toObject = toSnapshotTable.get(toObjectName);
if ((fromObject instanceof OmKeyInfo) && (toObject instanceof OmKeyInfo)) {
return !SnapshotDeletingService.isBlockLocationInfoSame(
(OmKeyInfo) fromObject, (OmKeyInfo) toObject);
} else if ((fromObject instanceof OmDirectoryInfo)
&& (toObject instanceof OmDirectoryInfo)) {
return !areAclsSame((OmDirectoryInfo) fromObject,
(OmDirectoryInfo) toObject);
} else {
throw new IllegalStateException("fromObject or toObject is not of " +
"the expected type. fromObject type : " +
fromObject.getClass().getName() + "toObject type: " +
toObject.getClass().getName());
}
}
private boolean areAclsSame(OmDirectoryInfo fromObject,
OmDirectoryInfo toObject) {
return fromObject.getAcls().equals(toObject.getAcls());
}

private boolean isBlockLocationSame(
String fromObjectName,
String toObjectName,
Expand Down
24 changes: 22 additions & 2 deletions ...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,21 @@ public static Codec<SnapshotDiffObject> getCodec() {
private String oldKeyName;
private String newKeyName;

private boolean isDirectory;

// Default constructor for Jackson Serializer.
public SnapshotDiffObject() {

}

public SnapshotDiffObject(long objectId,
String oldKeyName,
String newKeyName) {
String newKeyName,
boolean isDirectory) {
this.objectId = objectId;
this.oldKeyName = oldKeyName;
this.newKeyName = newKeyName;
this.isDirectory = isDirectory;
}

public long getObjectId() {
Expand Down Expand Up @@ -81,6 +85,14 @@ public void setNewKeyName(String newKeyName) {
this.newKeyName = newKeyName;
}

public void setDirectory(boolean directory) {
isDirectory = directory;
}

public boolean isDirectory() {
return isDirectory;
}

/**
* Builder for SnapshotDiffObject.
*/
Expand All @@ -89,6 +101,8 @@ public static class SnapshotDiffObjectBuilder {
private String oldKeyName;
private String newKeyName;

private boolean isDirectory;

public SnapshotDiffObjectBuilder(long objectID) {
this.objectId = objectID;
}
Expand All @@ -107,8 +121,14 @@ public SnapshotDiffObjectBuilder withNewKeyName(String keyName) {
return this;
}

public SnapshotDiffObjectBuilder setIsDirectory(boolean directory) {
isDirectory = directory;
return this;
}

public SnapshotDiffObject build() {
return new SnapshotDiffObject(objectId, oldKeyName, newKeyName);
return new SnapshotDiffObject(objectId, oldKeyName, newKeyName,
isDirectory);
}
}

Expand Down
16 changes: 11 additions & 5 deletions ...ne-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,9 +721,10 @@ public void testGenerateDiffReport() throws IOException {
toSnapName);

long totalDiffEntries = spy.generateDiffReport("jobId",
fromSnapTable, toSnapTable, objectIdToDiffObject, oldObjectIdKeyMap,
newObjectIdKeyMap, volumeName, bucketName, fromSnapName, toSnapName,
false, Optional.empty(), Optional.empty());
fromSnapTable, toSnapTable, null, null,
objectIdToDiffObject, oldObjectIdKeyMap, newObjectIdKeyMap, volumeName,
bucketName, fromSnapName, toSnapName, false, Optional.empty(),
Optional.empty());

assertEquals(100, totalDiffEntries);
SnapshotDiffJob snapshotDiffJob = new SnapshotDiffJob(0, "jobId",
Expand Down Expand Up @@ -1101,6 +1102,8 @@ public void testGenerateDiffReportWhenThereInEntry() {
long totalDiffEntries = snapshotDiffManager.generateDiffReport("jobId",
keyInfoTable,
keyInfoTable,
null,
null,
objectIdToDiffObject,
oldObjIdToKeyMap,
newObjIdToKeyMap,
Expand Down Expand Up @@ -1140,6 +1143,8 @@ public void testGenerateDiffReportFailure() throws IOException {
() -> spy.generateDiffReport("jobId",
keyInfoTable,
keyInfoTable,
null,
null,
objectIdToDiffObject,
oldObjIdToKeyMap,
newObjIdToKeyMap,
Expand Down Expand Up @@ -1411,8 +1416,9 @@ public void testGetSnapshotDiffReportHappyCase() throws Exception {
doNothing().when(spy).checkReportsIntegrity(any(), anyInt(), anyInt());

doReturn(10L).when(spy).generateDiffReport(anyString(),
any(), any(), any(), any(), any(), anyString(), anyString(),
anyString(), anyString(), anyBoolean(), any(), any());
any(), any(), any(), any(), any(), any(), any(),
anyString(), anyString(), anyString(), anyString(), anyBoolean(),
any(), any());
doReturn(LEGACY).when(spy).getBucketLayout(VOLUME_NAME, BUCKET_NAME,
omMetadataManager);

Expand Down