hemantk-12 commented on code in PR #4823:
URL: https://github.com/apache/ozone/pull/4823#discussion_r1235592112
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java:
##########
@@ -131,7 +132,7 @@ public class TestOmSnapshot {
private static AtomicInteger counter;
@Rule
- public Timeout timeout = new Timeout(180, TimeUnit.SECONDS);
+ public Timeout timeout = new Timeout(180, TimeUnit.HOURS);
Review Comment:
180 HOURS?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/snapshot/SnapshotDiffReportOzone.java:
##########
@@ -206,5 +209,46 @@ public void aggregate(SnapshotDiffReportOzone diffReport) {
this.getDiffList().addAll(diffReport.getDiffList());
}
+ /**
+ * DiffReportEntry for ozone.
+ */
+ public static class DiffReportEntryOzone extends DiffReportEntry {
+
+ public DiffReportEntryOzone(DiffType type, byte[] sourcePath) {
+ super(type, sourcePath);
+ }
+
+ public DiffReportEntryOzone(DiffType type, byte[][] sourcePathComponents) {
+ super(type, sourcePathComponents);
+ }
+
+ public DiffReportEntryOzone(DiffType type, byte[] sourcePath,
+ byte[] targetPath) {
+ super(type, sourcePath, targetPath);
+ }
+
+ public DiffReportEntryOzone(DiffType type, byte[][] sourcePathComponents,
+ byte[][] targetPathComponents) {
+ super(type, sourcePathComponents, targetPathComponents);
+ }
+
+ static String getPathString(byte[] path) {
+ String pathStr = DFSUtilClient.bytes2String(path);
+ return pathStr.isEmpty() ? "." : Paths.get(pathStr)
+ .toAbsolutePath().toString();
+ }
+
+ @Override
+ public String toString() {
+ String str = this.getType().getLabel() + "\t" +
+ getPathString(this.getSourcePath());
+ if (this.getType() == SnapshotDiffReport.DiffType.RENAME) {
+ str = str + " -> " + getPathString(this.getTargetPath());
+ }
+
+ return str;
+ }
+ }
+
Review Comment:
nit: please remove this extra line.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/FSODirectoryPathResolver.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.collect.Sets;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.hdds.utils.db.TableIterator;
+import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.Set;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
+/**
+ * Class to resolve absolute paths for FSO DirectoryInfo Objects.
+ */
+public class FSODirectoryPathResolver implements ObjectPathResolver {
+
+ private final String prefix;
+ private final long bucketId;
+ private final Table<String, OmDirectoryInfo> dirInfoTable;
+
+ public FSODirectoryPathResolver(String prefix, long bucketId,
+ Table<String, OmDirectoryInfo> dirInfoTable) {
+ this.prefix = prefix;
+ this.dirInfoTable = dirInfoTable;
+ this.bucketId = bucketId;
+ }
+
+ private void addToPathMap(Pair<Long, Path> objectIDPath,
+ Set<Long> dirObjIds, Map<Long, Path> pathMap) {
+ if (dirObjIds.contains(objectIDPath.getKey())) {
+ pathMap.put(objectIDPath.getKey(), objectIDPath.getValue());
+ dirObjIds.remove(objectIDPath.getKey());
+ }
+ }
+
+ /**
+ * Assuming all dirObjIds belong to a bucket this function resolves absolute
+ * path for a given FSO bucket.
+ * @param dirObjIds
+ * @return Map of Path corresponding to provided directory object IDs
+ */
+ @SuppressFBWarnings("DMI_HARDCODED_ABSOLUTE_FILENAME")
Review Comment:
Instead of suppressing the warning, you could have created a constant for it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -1194,23 +1241,28 @@ private boolean areKeysEqual(WithObjectID oldKey,
WithObjectID newKey) {
}
/**
- * check if the given key is in the bucket specified by tablePrefix map.
+ * Get table prefix given a tableName.
*/
- boolean isKeyInBucket(String key, Map<String, String> tablePrefixes,
- String tableName) {
+ private String getTablePrefix(Map<String, String> tablePrefixes,
+ String tableName) {
Review Comment:
Alignment is bit off.
```suggestion
private String getTablePrefix(Map<String, String> tablePrefixes,
String tableName) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java:
##########
@@ -737,15 +747,32 @@ private void generateSnapshotDiffReport(final String
jobKey,
fromSnapshot, toSnapshot, fsInfo, tsInfo, useFullDiff,
tablePrefixes, objectIdToKeyNameMapForFromSnapshot,
objectIdToKeyNameMapForToSnapshot, objectIDsToCheckMap,
- path.toString());
+ oldParentIds, newParentIds, path.toString());
}
validateSnapshotsAreActive(volumeName, bucketName, fromSnapshotName,
toSnapshotName);
- long totalDiffEntries = generateDiffReport(jobId,
- objectIDsToCheckMap,
+ Map<Long, Path> oldParentIdPathMap = null;
+ Map<Long, Path> newParentIdPathMap = null;
Review Comment:
When you mark a comment done and resolved, it should be either addressed or
have a reply.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java:
##########
@@ -208,7 +209,7 @@ private void init() throws Exception {
// stop the deletion services so that keys can still be read
keyManager.stop();
- preFinalizationChecks();
+// preFinalizationChecks();
Review Comment:
Is it intentional?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/snapshot/SnapshotDiffReportOzone.java:
##########
@@ -206,5 +209,46 @@ public void aggregate(SnapshotDiffReportOzone diffReport) {
this.getDiffList().addAll(diffReport.getDiffList());
}
+ /**
+ * DiffReportEntry for ozone.
+ */
+ public static class DiffReportEntryOzone extends DiffReportEntry {
Review Comment:
What are you trying to achieve with `DiffReportEntryOzone`? I don't see
anything is overridden in `DiffReportEntryOzone`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshot.java:
##########
@@ -591,12 +592,11 @@ public void testSnapDiff() throws Exception {
SnapshotDiffReportOzone
diff2 = getSnapDiffReport(volume, bucket, snap2, snap3);
Assert.assertEquals(2, diff2.getDiffList().size());
- Assert.assertTrue(diff2.getDiffList().contains(
- SnapshotDiffReportOzone.getDiffReportEntry(
- SnapshotDiffReportOzone.DiffType.CREATE, key2)));
- Assert.assertTrue(diff2.getDiffList().contains(
- SnapshotDiffReportOzone.getDiffReportEntry(
- SnapshotDiffReportOzone.DiffType.DELETE, key1)));
+ Assert.assertEquals(diff2.getDiffList(),
Review Comment:
1. It should be the other way `Assert.assertEquals(expected, actual);`
```suggestion
Assert.assertEquals(
Arrays.asList(SnapshotDiffReportOzone.getDiffReportEntry(
SnapshotDiffReport.DiffType.DELETE, "/" + key1),
SnapshotDiffReportOzone.getDiffReportEntry(
SnapshotDiffReport.DiffType.CREATE, "/" + key2)),
diff2.getDiffList());
```
2. Use `OM_KEY_PREFIX` instead of `/`.
3. Curious if it should be `./key`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/FSODirectoryPathResolver.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.collect.Sets;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.hdds.utils.db.TableIterator;
+import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.Set;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
+/**
+ * Class to resolve absolute paths for FSO DirectoryInfo Objects.
+ */
+public class FSODirectoryPathResolver implements ObjectPathResolver {
+
+ private final String prefix;
+ private final long bucketId;
+ private final Table<String, OmDirectoryInfo> dirInfoTable;
+
+ public FSODirectoryPathResolver(String prefix, long bucketId,
+ Table<String, OmDirectoryInfo> dirInfoTable) {
+ this.prefix = prefix;
+ this.dirInfoTable = dirInfoTable;
+ this.bucketId = bucketId;
+ }
+
+ private void addToPathMap(Pair<Long, Path> objectIDPath,
+ Set<Long> dirObjIds, Map<Long, Path> pathMap) {
+ if (dirObjIds.contains(objectIDPath.getKey())) {
+ pathMap.put(objectIDPath.getKey(), objectIDPath.getValue());
+ dirObjIds.remove(objectIDPath.getKey());
+ }
+ }
+
+ /**
+ * Assuming all dirObjIds belong to a bucket this function resolves absolute
+ * path for a given FSO bucket.
+ * @param dirObjIds
+ * @return Map of Path corresponding to provided directory object IDs
+ */
+ @SuppressFBWarnings("DMI_HARDCODED_ABSOLUTE_FILENAME")
+ @Override
+ public Map<Long, Path> getAbsolutePathForObjectIDs(
+ Optional<Set<Long>> dirObjIds) throws IOException {
+ // Root of a bucket would always have the
+ // key as /volumeId/bucketId/bucketId/
+ if (!dirObjIds.isPresent() || dirObjIds.get().isEmpty()) {
+ return Collections.emptyMap();
+ }
+ Set<Long> objIds = Sets.newHashSet(dirObjIds.get());
+ Map<Long, Path> pathMap = new HashMap<>();
Review Comment:
nit:
```suggestion
Map<Long, Path> objectIdToPathMap = new HashMap<>();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/FSODirectoryPathResolver.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.ozone.om.snapshot;
+
+import com.google.common.collect.Sets;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.hdds.utils.db.TableIterator;
+import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Queue;
+import java.util.Set;
+
+import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
+
+/**
+ * Class to resolve absolute paths for FSO DirectoryInfo Objects.
+ */
+public class FSODirectoryPathResolver implements ObjectPathResolver {
+
+ private final String prefix;
+ private final long bucketId;
+ private final Table<String, OmDirectoryInfo> dirInfoTable;
+
+ public FSODirectoryPathResolver(String prefix, long bucketId,
+ Table<String, OmDirectoryInfo> dirInfoTable) {
+ this.prefix = prefix;
+ this.dirInfoTable = dirInfoTable;
+ this.bucketId = bucketId;
+ }
+
+ private void addToPathMap(Pair<Long, Path> objectIDPath,
+ Set<Long> dirObjIds, Map<Long, Path> pathMap) {
+ if (dirObjIds.contains(objectIDPath.getKey())) {
+ pathMap.put(objectIDPath.getKey(), objectIDPath.getValue());
+ dirObjIds.remove(objectIDPath.getKey());
+ }
+ }
+
+ /**
+ * Assuming all dirObjIds belong to a bucket this function resolves absolute
+ * path for a given FSO bucket.
+ * @param dirObjIds
Review Comment:
nit: either remove this or add the description.
--
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]
