hemantk-12 commented on code in PR #4125:
URL: https://github.com/apache/ozone/pull/4125#discussion_r1084560530
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java:
##########
@@ -343,6 +349,35 @@ public void ensureKeyAddress() throws OzoneClientException
{
}
}
+ /**
+ * Similar to #ensureBucketAddress()
+ * but also accepting a snapshot
+ * indicator and a snapshot name.
+ * If the keyName can't be considered
+ * a valid snapshot, an exception is thrown.
+ *
+ * @throws OzoneClientException
+ */
+ public void ensureSnapshotAddress()
+ throws OzoneClientException {
+ if (keyName.length() > 0) {
+ if (OmUtils.isBucketSnapshotIndicator(keyName)) {
+ snapshotName += keyName;
Review Comment:
I think this is wrong to just append `keyName` to `snapshotName` because key
name contains `SNAPSHOT_INDICATOR`= `.snapshot`. May be use better name or
extract out the snapshotName.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/OzoneAddress.java:
##########
@@ -343,6 +349,35 @@ public void ensureKeyAddress() throws OzoneClientException
{
}
}
+ /**
+ * Similar to #ensureBucketAddress()
Review Comment:
nit:
1. I don't think it is a good idea to mention `Similar to XYZ` as starting
of Java doc comment.
2. Method name says that ensuring snapshot address but it does more than
just ensuring snapshot address.
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneAddress.java:
##########
@@ -47,26 +48,34 @@ public static Collection<Object[]> data() {
});
}
- private String prefix;
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ private final String prefix;
+ private OzoneAddress address;
public TestOzoneAddress(String prefix) {
this.prefix = prefix;
}
@Test
- public void checkUrlTypes() throws OzoneClientException, IOException {
- OzoneAddress address;
-
+ public void checkRootUrlType() throws OzoneClientException {
address = new OzoneAddress("");
Review Comment:
What's the difference between this and last parameter in line 47?
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneAddress.java:
##########
@@ -96,14 +108,37 @@ public void checkUrlTypes() throws OzoneClientException,
IOException {
Assert.assertEquals("key1/key3/key", address.getKeyName());
Assert.assertFalse("this should not be a prefix",
address.isPrefix());
+ }
+ @Test
+ public void checkPrefixUrlType() throws OzoneClientException {
address = new OzoneAddress(prefix + "vol1/bucket/prefix");
address.ensurePrefixAddress();
Assert.assertEquals("vol1", address.getVolumeName());
Assert.assertEquals("bucket", address.getBucketName());
Assert.assertEquals("prefix", address.getKeyName());
Assert.assertTrue("this should be a prefix",
address.isPrefix());
+ }
+
+ @Test
+ public void checkSnapshotUrlType() throws OzoneClientException {
+ address = new OzoneAddress(prefix + "vol1/bucket/.snapshot/snap1");
+ address.ensureSnapshotAddress();
+ Assert.assertEquals("vol1", address.getVolumeName());
+ Assert.assertEquals("bucket", address.getBucketName());
+ Assert.assertEquals(".snapshot/snap1", address.getSnapshotName());
+ Assert.assertEquals(".snapshot/snap1", address.getKeyName());
+
+
+ String message = "Only a snapshot name with " +
+ "a snapshot indicator is accepted";
+
+ address = new OzoneAddress(prefix + "vol1/bucket/.snapshot");
+
+ exception.expect(OzoneClientException.class);
+ exception.expectMessage(message);
+ address.ensureSnapshotAddress();
Review Comment:
```suggestion
OzoneClientException exception =
assertThrows(OzoneClientException.class, () -> {
address.ensureSnapshotAddress();
});
assertTrue(exception.getMessage().contains(message));
```
It would be nicer and simpler if you use Junit5.
https://www.baeldung.com/junit-assert-exception
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/ListKeyHandler.java:
##########
@@ -21,25 +21,26 @@
import java.io.IOException;
import java.util.Iterator;
+import com.google.common.base.Strings;
import org.apache.hadoop.ozone.client.OzoneBucket;
import org.apache.hadoop.ozone.client.OzoneClient;
import org.apache.hadoop.ozone.client.OzoneClientException;
import org.apache.hadoop.ozone.client.OzoneKey;
import org.apache.hadoop.ozone.client.OzoneVolume;
import org.apache.hadoop.ozone.shell.ListOptions;
import org.apache.hadoop.ozone.shell.OzoneAddress;
-import org.apache.hadoop.ozone.shell.bucket.BucketHandler;
+import org.apache.hadoop.ozone.shell.snapshot.SnapshotHandler;
import picocli.CommandLine;
import picocli.CommandLine.Command;
/**
- * Executes List Keys.
+ * Executes List Keys for a bucket or snapshot.
*/
@Command(name = "list",
aliases = "ls",
- description = "list all keys in a given bucket")
-public class ListKeyHandler extends BucketHandler {
+ description = "list all keys in a given bucket or snapshot")
+public class ListKeyHandler extends SnapshotHandler {
Review Comment:
I don't think we need to create `SnapshotHandler` because it is still
listing keys from snapshot bucket.
Also structurally `SnapshotHandler` gives the wrong impression. Either it is
to handle snapshot creation request or snapshot listing request not list keys
from snapshotted bucket.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/keys/ListKeyHandler.java:
##########
@@ -50,11 +51,27 @@ protected void execute(OzoneClient client, OzoneAddress
address)
String volumeName = address.getVolumeName();
String bucketName = address.getBucketName();
+ String snapshotName = "";
Review Comment:
```suggestion
String snapshotName = address.getSnapshotName();
String keyPrefix = "";
if (!Strings.isNullOrEmpty(snapshotName)) {
keyPrefix += snapshotName;
if (!Strings.isNullOrEmpty(listOptions.getPrefix())) {
keyPrefix += "/";
}
}
if (!Strings.isNullOrEmpty(listOptions.getPrefix())) {
keyPrefix += listOptions.getPrefix();
}
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneAddress.java:
##########
@@ -47,26 +48,34 @@ public static Collection<Object[]> data() {
});
}
- private String prefix;
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ private final String prefix;
+ private OzoneAddress address;
public TestOzoneAddress(String prefix) {
this.prefix = prefix;
}
@Test
- public void checkUrlTypes() throws OzoneClientException, IOException {
- OzoneAddress address;
-
+ public void checkRootUrlType() throws OzoneClientException {
address = new OzoneAddress("");
address.ensureRootAddress();
address = new OzoneAddress(prefix + "");
address.ensureRootAddress();
+ }
+ @Test
Review Comment:
You can also upgrade these tests to Junit5.
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneAddress.java:
##########
@@ -96,14 +108,37 @@ public void checkUrlTypes() throws OzoneClientException,
IOException {
Assert.assertEquals("key1/key3/key", address.getKeyName());
Assert.assertFalse("this should not be a prefix",
address.isPrefix());
+ }
+ @Test
+ public void checkPrefixUrlType() throws OzoneClientException {
address = new OzoneAddress(prefix + "vol1/bucket/prefix");
address.ensurePrefixAddress();
Assert.assertEquals("vol1", address.getVolumeName());
Assert.assertEquals("bucket", address.getBucketName());
Assert.assertEquals("prefix", address.getKeyName());
Assert.assertTrue("this should be a prefix",
address.isPrefix());
+ }
+
+ @Test
+ public void checkSnapshotUrlType() throws OzoneClientException {
+ address = new OzoneAddress(prefix + "vol1/bucket/.snapshot/snap1");
+ address.ensureSnapshotAddress();
+ Assert.assertEquals("vol1", address.getVolumeName());
+ Assert.assertEquals("bucket", address.getBucketName());
+ Assert.assertEquals(".snapshot/snap1", address.getSnapshotName());
+ Assert.assertEquals(".snapshot/snap1", address.getKeyName());
+
+
+ String message = "Only a snapshot name with " +
+ "a snapshot indicator is accepted";
+
+ address = new OzoneAddress(prefix + "vol1/bucket/.snapshot");
+
+ exception.expect(OzoneClientException.class);
+ exception.expectMessage(message);
+ address.ensureSnapshotAddress();
}
}
Review Comment:
Please add a new line at the end.
--
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]
