hemantk-12 commented on code in PR #4134:
URL: https://github.com/apache/ozone/pull/4134#discussion_r1081672581
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java:
##########
@@ -24,33 +25,38 @@
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
import org.apache.hadoop.util.ToolRunner;
-import org.junit.AfterClass;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.BeforeClass;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.Timeout;
+import org.apache.ozone.test.GenericTestUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
import static org.apache.hadoop.fs.FileSystem.FS_DEFAULT_NAME_KEY;
import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_OFS_URI_SCHEME;
-import static org.junit.Assert.assertEquals;
+import static org.apache.hadoop.ozone.OzoneConsts.OM_SNAPSHOT_INDICATOR;
/**
* Test client-side CRUD snapshot operations with Ozone Manager.
+ * Setting a timeout for every test method to 300 seconds.
*/
+@Timeout(value = 300)
public class TestOzoneFsSnapshot {
- // Set the timeout for every test.
- @Rule
- public Timeout testTimeout = Timeout.seconds(300);
private static MiniOzoneCluster cluster;
private static final String OM_SERVICE_ID = "om-service-test1";
- private OzoneConfiguration clientConf;
private static OzoneManager ozoneManager;
-
- @BeforeClass
+ private static OzoneFsShell shell;
+ private static String volume;
+ private static String bucket;
+ private static String key;
+ private static String bucketPath;
+ private static String bucketWithSnapshotIndicatorPath;
+
+ @BeforeAll
Review Comment:
+1 on upgrading to Junit-5.
It would be great if you could change these tests into two parameterized
tests, valid scenarios and invalid scenarios.
https://www.baeldung.com/parameterized-tests-junit-5 and
https://github.com/apache/ozone/blob/HDDS-6517-Snapshot/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/rocksdiff/TestRocksDBCheckpointDiffer.java#L196
##########
hadoop-ozone/dist/src/main/smoketest/snapshot/fs-snapshot.robot:
##########
@@ -0,0 +1,96 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
Review Comment:
Should we merge this and https://github.com/apache/ozone/pull/4171?
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -782,6 +791,32 @@ private List<FileStatusAdapter> listStatusVolume(String
volumeStr,
return res;
}
+ /**
+ * Helper for OFS listStatus
+ * on a bucket to get all snapshots.
+ */
+ private List<FileStatusAdapter> listStatusBucketSnapshot(
+ String volumeName, String bucketName, URI uri) throws IOException {
+ List<OzoneSnapshot> snapshotList =
+ objectStore.listSnapshot(volumeName, bucketName);
+
+ OzoneVolume volume = objectStore.getVolume(volumeName);
+ UserGroupInformation ugi =
+ UserGroupInformation.createRemoteUser(volume.getOwner());
+ String owner = ugi.getShortUserName();
+ String group = getGroupName(ugi);
+
+ OzoneBucket ozoneBucket = getBucket(volumeName, bucketName, false);
+ List<FileStatusAdapter> fileStatusAdapterList = new ArrayList<>();
+ for (OzoneSnapshot ozoneSnapshot : snapshotList) {
+ fileStatusAdapterList.add(
+ getFileStatusAdapterForBucketSnapshot(
+ ozoneBucket, ozoneSnapshot, uri, owner, group));
+ }
+
+ return fileStatusAdapterList;
Review Comment:
```suggestion
return snapshots.stream()
.map(ozoneSnapshot -> getFileStatusAdapterForBucketSnapshot(
ozoneBucket, ozoneSnapshot, uri, owner, group))
.collect(Collectors.toList());
```
nit: you can use lambdas.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -263,6 +264,23 @@ public boolean isBucket() {
!this.getVolumeName().isEmpty();
}
+ /**
+ * If volume and bucket names are not empty and the key name
+ * only contains the snapshot indicator, then return true.
+ * e.g. /vol/bucket/.snapshot is a snapshot indicator.
+ */
+ public boolean isSnapshotIndicator() {
Review Comment:
```suggestion
public boolean isSnapshotDirectory() {
```
nit: I think `isSnapshotDirectory` would be a better name for this function.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -643,6 +645,13 @@ public FileStatusAdapter getFileStatus(String path, URI
uri,
}
try {
OzoneBucket bucket = getBucket(ofsPath, false);
+
+ if (ofsPath.isSnapshotIndicator()) {
Review Comment:
It would be easier to read if written like below and exception is handle in
individual function.
```
if (ofsPath.isRoot()) {
return getFileStatusAdapterForRoot(uri);
} else if (ofsPath.isVolume()) {
OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
return getFileStatusAdapterForVolume(volume, uri);
} else if (ofsPath.isSnapshotIndicator()) {
OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
return getFileStatusAdapterWithSnapshotIndicator(
volume, bucket, uri);
} else {
OzoneFileStatus status = bucket.getFileStatus(key);
return toFileStatusAdapter(status, userName, uri, qualifiedPath,
ofsPath.getNonKeyPath());
}
```
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -782,6 +791,32 @@ private List<FileStatusAdapter> listStatusVolume(String
volumeStr,
return res;
}
+ /**
+ * Helper for OFS listStatus
Review Comment:
```suggestion
* Helper for OFS listStatus on a bucket to get all snapshots.
```
Could be a one line.
--
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]
