hemantk-12 commented on code in PR #4134:
URL: https://github.com/apache/ozone/pull/4134#discussion_r1087190976
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java:
##########
@@ -159,63 +141,53 @@ public void testCreateSnapshotDuplicateName() throws
Exception {
Assertions.assertEquals(1, res);
}
- @Test
- public void testCreateSnapshotInvalidName() throws Exception {
- String snapshotName = "snapa?b";
-
- int res = ToolRunner.run(shell,
- new String[]{"-createSnapshot", bucketPath, snapshotName});
- // Asserts that create request failed since invalid name passed
- Assertions.assertEquals(1, res);
- }
-
- @Test
- public void testCreateSnapshotOnlyNumericName() throws Exception {
- String snapshotName = "1234";
-
+ /**
+ * Create snapshot should succeed.
+ * 1st case: valid snapshot name
+ * 2nd case: snapshot name length is less than 64 chars
+ */
+ @ParameterizedTest
+ @ValueSource(strings = {"snap-1",
+ "snap75795657617173401188448010125899089001363595171500499231286"})
+ public void testCreateSnapshotSuccess(String snapshotName)
+ throws Exception {
int res = ToolRunner.run(shell,
new String[]{"-createSnapshot", bucketPath, snapshotName});
- // Asserts that create request failed since only numeric name passed
- Assertions.assertEquals(1, res);
- }
-
- @Test
- public void testCreateSnapshotInvalidURI() throws Exception {
-
- int res = ToolRunner.run(shell,
- new String[]{"-createSnapshot", "invalidURI"});
- // Asserts that create request failed since
- // invalid volume-bucket URI passed
- Assertions.assertEquals(1, res);
- }
-
- @Test
- public void testCreateSnapshotNameLength() throws Exception {
- String name63 =
- "snap75795657617173401188448010125899089001363595171500499231286";
- String name64 =
- "snap156808943643007724443266605711479126926050896107709081166294";
-
- int res = ToolRunner.run(shell,
- new String[]{"-createSnapshot", bucketPath, name63});
- // Asserts that create request succeeded since name length
- // less than 64 char
+ // Asserts that create request succeeded
Assertions.assertEquals(0, res);
- res = ToolRunner.run(shell,
- new String[]{"-createSnapshot", bucketPath, name64});
- // Asserts that create request fails since name length
- // more than 64 char
- Assertions.assertEquals(1, res);
-
SnapshotInfo snapshotInfo = ozoneManager
.getMetadataManager()
.getSnapshotInfoTable()
- .get(SnapshotInfo.getTableKey(volume, bucket, name63));
+ .get(SnapshotInfo.getTableKey(volume, bucket, snapshotName));
+ // Assert that snapshot exists in RocksDB.
+ // We can't use list or valid if snapshot directory exists because DB
+ // transaction might not be flushed by the time.
Assertions.assertNotNull(snapshotInfo);
}
+ /**
+ * Create snapshot should fail.
+ * 1st case: snapshot name contains invalid char
+ * 2nd case: snapshot name consists only of numbers
+ * 3rd case: bucket path is invalid
+ * 4th case: snapshot name length is more than 64 chars
+ */
+ @ParameterizedTest
+ @ValueSource(strings = {"snapa?b", "1234", "invalidURI",
+ "snap156808943643007724443266605711479126926050896107709081166294"})
+ public void testCreateSnapshotFailure(String snapshotName)
+ throws Exception {
+ if (snapshotName.equals("invalidURI")) {
Review Comment:
nit: you could have pass `bucketPath` as well instead of tweaking it based
on `snapshotName`.
##########
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:
Don't have strong point to move try-catch to individual function. One thing
it will improve is nesting. Right now we have too much nesting.
##########
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:
May be you can use something more generic like `isShanshotAddress` or more
relevant. But `isSnapshotIndicator()` is bit a confusing to me.
##########
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:
Sounds good to me.
--
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]
