umamaheswararao commented on a change in pull request #906:
URL: https://github.com/apache/hadoop-ozone/pull/906#discussion_r424867606
##########
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -469,15 +478,72 @@ public boolean delete(Path f, boolean recursive) throws
IOException {
return false;
}
+ OFSPath ofsPath = new OFSPath(key);
+
Review comment:
Before this there is a root validation.
if (key.equals("/")) {
LOG.warn("Cannot delete root directory.");
return false;
}
Is this above check still needed?
##########
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -469,15 +478,72 @@ public boolean delete(Path f, boolean recursive) throws
IOException {
return false;
}
+ OFSPath ofsPath = new OFSPath(key);
+
+ // Handle rm root
+ if (ofsPath.isRoot()) {
+ // Intentionally drop support for rm root
+ // because it is too dangerous and doesn't provide much value
+ LOG.warn("delete: OFS does not support rm root. "
+ + "To wipe the cluster, please re-init OM instead.");
+ return false;
+ }
+
+ // Handle delete volume
+ if (ofsPath.isVolume()) {
+ String volumeName = ofsPath.getVolumeName();
+ if (recursive) {
+ // Delete all buckets first
+ OzoneVolume volume =
+ adapterImpl.getObjectStore().getVolume(volumeName);
+ Iterator<? extends OzoneBucket> it = volume.listBuckets("");
+ while (it.hasNext()) {
+ OzoneBucket bucket = it.next();
Review comment:
I think we can do this addTrailingSlashIfNeeded(f.toString()) outside of
loop once?
path.toString also does lot of conversions. we can avoid that if we do it
outside loop
##########
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -469,15 +478,72 @@ public boolean delete(Path f, boolean recursive) throws
IOException {
return false;
}
+ OFSPath ofsPath = new OFSPath(key);
+
+ // Handle rm root
+ if (ofsPath.isRoot()) {
+ // Intentionally drop support for rm root
+ // because it is too dangerous and doesn't provide much value
+ LOG.warn("delete: OFS does not support rm root. "
+ + "To wipe the cluster, please re-init OM instead.");
+ return false;
+ }
+
+ // Handle delete volume
+ if (ofsPath.isVolume()) {
+ String volumeName = ofsPath.getVolumeName();
+ if (recursive) {
+ // Delete all buckets first
+ OzoneVolume volume =
+ adapterImpl.getObjectStore().getVolume(volumeName);
+ Iterator<? extends OzoneBucket> it = volume.listBuckets("");
+ while (it.hasNext()) {
+ OzoneBucket bucket = it.next();
+ String nextBucket = addTrailingSlashIfNeeded(
+ f.toString()) + bucket.getName();
+ delete(new Path(nextBucket), true);
+ }
+ }
+ try {
+ adapterImpl.getObjectStore().deleteVolume(volumeName);
+ return true;
+ } catch (OMException ex) {
+ // volume is not empty
+ if (ex.getResult() == VOLUME_NOT_EMPTY) {
+ throw new PathIsNotEmptyDirectoryException(f.toString());
+ } else {
+ throw ex;
+ }
+ }
+ }
+
result = innerDelete(f, recursive);
+
+ // Handle delete bucket
+ if (ofsPath.isBucket()) {
+ OzoneVolume volume =
+ adapterImpl.getObjectStore().getVolume(ofsPath.getVolumeName());
+ try {
+ volume.deleteBucket(ofsPath.getBucketName());
+ return result;
+ } catch (OMException ex) {
+ // bucket is not empty
+ if (ex.getResult() == BUCKET_NOT_EMPTY) {
+ throw new PathIsNotEmptyDirectoryException(f.toString());
+ } else {
+ throw ex;
+ }
+ }
+ }
+
} else {
LOG.debug("delete: Path is a file: {}", f);
result = adapter.deleteObject(key);
}
if (result) {
// If this delete operation removes all files/directories from the
- // parent direcotry, then an empty parent directory must be created.
+ // parent directory, then an empty parent directory must be created.
Review comment:
Why is this? Do you mind explaining why do we need to create parent dir.
Parent dir should not have deleted right? I know its not related to this
change, but just for my understanding.
##########
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java
##########
@@ -457,6 +462,10 @@ public boolean delete(Path f, boolean recursive) throws
IOException {
return false;
}
+ if (status == null) {
+ return false;
+ }
+
String key = pathToKey(f);
boolean result;
Review comment:
Can we add javadoc to delete method here? parent abstract method delete
does not provide any javadoc. SO, its good to provide here and explain the
behavior?
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -766,4 +768,103 @@ public void testTempMount() throws Exception {
fileStatusesInDir1[0].getPath().toUri().getPath());
}
+ /**
+ * Helper function. Check Ozone volume existence.
+ * @param volumeStr Name of the volume
+ * @return true if volume exists, false if not
+ */
+ private boolean volumeExist(String volumeStr) throws IOException {
+ try {
+ objectStore.getVolume(volumeStr);
+ } catch (OMException ex) {
+ if (ex.getResult() == VOLUME_NOT_FOUND) {
+ return false;
+ } else {
+ throw ex;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Helper function. Delete a path non-recursively and expect failure.
+ * @param f Path to delete.
+ * @throws IOException
+ */
+ private void deleteNonRecursivelyAndFail(Path f) throws IOException {
+ try {
+ fs.delete(f, false);
+ Assert.fail("Should have thrown PathIsNotEmptyDirectoryException!");
+ } catch (PathIsNotEmptyDirectoryException ignored) {
+ }
+ }
+
+ @Test
+ public void testDeleteVolumeAndBucket() throws IOException {
Review comment:
I think these test cases should be split into multiple cases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
