[
https://issues.apache.org/jira/browse/HADOOP-19446?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17930294#comment-17930294
]
ASF GitHub Bot commented on HADOOP-19446:
-----------------------------------------
anujmodi2021 commented on code in PR #7376:
URL: https://github.com/apache/hadoop/pull/7376#discussion_r1969592544
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -359,6 +494,11 @@ public void testDeleteImplicitDir() throws Exception {
AbfsBlobClient client = (AbfsBlobClient) fs.getAbfsClient();
client.deleteBlobPath(new Path("/testDir/dir1"),
null, getTestTracingContext(fs, true));
+
+ //Deleting non-empty dir with recursion set as
Review Comment:
Why this needed?
Also the test name says deleting implicit dir but it seems like it is
deleting explicit dir only.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -344,6 +363,122 @@ public void
deleteBlobDirParallelThreadToDeleteOnDifferentTracingContext()
fs.close();
}
+ /**
+ * Test the deletion of file in an implicit directory.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteFileInImplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+ assumeBlobServiceType();
+
+ Path p = new Path("/testDir/dir1");
+ Path p1 = new Path("/testDir/dir1/file1");
+ Path p2 = new Path("/testDir/dir1/file2");
+
+ fs.create(p1);
+ fs.create(p2);
+ AbfsBlobClient client = (AbfsBlobClient) fs.getAbfsClient();
+ client.deleteBlobPath(p, null,
+ getTestTracingContext(fs, true));
+
+ // Deletion of file with different recursion values
+ fs.delete(p1, false);
+ fs.delete(p2, true);
+
+ Assertions.assertThat(fs.exists(p))
+ .describedAs("The directory should exist.")
+ .isTrue();
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("Deleted file should not be present.").isFalse();
+ Assertions.assertThat(fs.exists(p2))
+ .describedAs("Deleted file should not be present.").isFalse();
+ }
+
+ /**
+ * Test that the file status of an empty explicit dir
+ * should not exist after its deletion.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1/");
+
+ fs.mkdirs(p1);
+ fs.delete(p1, false);
+
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("The deleted directory should not exist.")
+ .isFalse();
+ }
+
+ /**
+ * Test that deleting a non-empty explicit directory
+ * can only be done with the recursive flag set to true.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteNonEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1");
+ Path p2 = new Path("/testDir2");
+
+ fs.create(new Path("/testDir1/f1.txt"));
+ fs.create(new Path("/testDir2/f2.txt"));
+
+ fs.delete(p1, true);
+
+ //Deleting non-empty dir with recursion set as
+ // false returns a FileAlreadyExistsException: 409-DirectoryNotEmpty
+ intercept(FileAlreadyExistsException.class,
+ () -> fs.delete(p2, false));
+
+ Assertions.assertThat(!fs.exists(p1))
+ .describedAs("FileStatus of the deleted directory should not exist.")
+ .isTrue();
+ Assertions.assertThat(!fs.exists(p1))
+ .describedAs("FileStatus of the deleted directory should not exist.")
+ .isTrue();
+ }
+
+ /**
+ * Assert that deleting a non-existing path
+ * leads to a FileNotFoundException.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteNonExistingPath() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p = new Path("/nonExistingPath");
+ intercept(FileNotFoundException.class,
+ () -> assertDeleted(fs, p, true));
Review Comment:
can we simply call fs.delete() here?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -344,6 +363,122 @@ public void
deleteBlobDirParallelThreadToDeleteOnDifferentTracingContext()
fs.close();
}
+ /**
+ * Test the deletion of file in an implicit directory.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteFileInImplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+ assumeBlobServiceType();
+
+ Path p = new Path("/testDir/dir1");
+ Path p1 = new Path("/testDir/dir1/file1");
+ Path p2 = new Path("/testDir/dir1/file2");
+
+ fs.create(p1);
+ fs.create(p2);
+ AbfsBlobClient client = (AbfsBlobClient) fs.getAbfsClient();
+ client.deleteBlobPath(p, null,
Review Comment:
How is this delete on a file in implicit dir?
fs.create() will create explicit dir right?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -344,6 +363,122 @@ public void
deleteBlobDirParallelThreadToDeleteOnDifferentTracingContext()
fs.close();
}
+ /**
+ * Test the deletion of file in an implicit directory.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteFileInImplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+ assumeBlobServiceType();
+
+ Path p = new Path("/testDir/dir1");
+ Path p1 = new Path("/testDir/dir1/file1");
+ Path p2 = new Path("/testDir/dir1/file2");
+
+ fs.create(p1);
+ fs.create(p2);
+ AbfsBlobClient client = (AbfsBlobClient) fs.getAbfsClient();
+ client.deleteBlobPath(p, null,
+ getTestTracingContext(fs, true));
+
+ // Deletion of file with different recursion values
+ fs.delete(p1, false);
+ fs.delete(p2, true);
+
+ Assertions.assertThat(fs.exists(p))
+ .describedAs("The directory should exist.")
+ .isTrue();
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("Deleted file should not be present.").isFalse();
+ Assertions.assertThat(fs.exists(p2))
+ .describedAs("Deleted file should not be present.").isFalse();
+ }
+
+ /**
+ * Test that the file status of an empty explicit dir
+ * should not exist after its deletion.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1/");
+
+ fs.mkdirs(p1);
+ fs.delete(p1, false);
+
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("The deleted directory should not exist.")
+ .isFalse();
+ }
+
+ /**
+ * Test that deleting a non-empty explicit directory
+ * can only be done with the recursive flag set to true.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteNonEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1");
+ Path p2 = new Path("/testDir2");
+
+ fs.create(new Path("/testDir1/f1.txt"));
+ fs.create(new Path("/testDir2/f2.txt"));
+
+ fs.delete(p1, true);
+
+ //Deleting non-empty dir with recursion set as
+ // false returns a FileAlreadyExistsException: 409-DirectoryNotEmpty
+ intercept(FileAlreadyExistsException.class,
+ () -> fs.delete(p2, false));
+
+ Assertions.assertThat(!fs.exists(p1))
+ .describedAs("FileStatus of the deleted directory should not exist.")
+ .isTrue();
+ Assertions.assertThat(!fs.exists(p1))
+ .describedAs("FileStatus of the deleted directory should not exist.")
+ .isTrue();
+ }
+
+ /**
+ * Assert that deleting a non-existing path
+ * leads to a FileNotFoundException.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteNonExistingPath() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p = new Path("/nonExistingPath");
+ intercept(FileNotFoundException.class,
+ () -> assertDeleted(fs, p, true));
+ }
+
+ /**
+ * Test to check if fileNotFoundException is
+ * injected after the file has already been deleted.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testExceptionForDeletedFile() throws Exception {
+ final AzureBlobFileSystem fs = getFileSystem();
+ Path testFile = path("/testFile");
+ fs.create(testFile);
+ fs.delete(testFile, false);
+
+ intercept(FileNotFoundException.class,
+ () -> assertDeleted(fs, testFile, true));
Review Comment:
Same as above.
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -344,6 +363,122 @@ public void
deleteBlobDirParallelThreadToDeleteOnDifferentTracingContext()
fs.close();
}
+ /**
+ * Test the deletion of file in an implicit directory.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteFileInImplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+ assumeBlobServiceType();
+
+ Path p = new Path("/testDir/dir1");
+ Path p1 = new Path("/testDir/dir1/file1");
+ Path p2 = new Path("/testDir/dir1/file2");
+
+ fs.create(p1);
+ fs.create(p2);
+ AbfsBlobClient client = (AbfsBlobClient) fs.getAbfsClient();
+ client.deleteBlobPath(p, null,
+ getTestTracingContext(fs, true));
+
+ // Deletion of file with different recursion values
+ fs.delete(p1, false);
+ fs.delete(p2, true);
+
+ Assertions.assertThat(fs.exists(p))
+ .describedAs("The directory should exist.")
+ .isTrue();
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("Deleted file should not be present.").isFalse();
+ Assertions.assertThat(fs.exists(p2))
+ .describedAs("Deleted file should not be present.").isFalse();
+ }
+
+ /**
+ * Test that the file status of an empty explicit dir
+ * should not exist after its deletion.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1/");
+
+ fs.mkdirs(p1);
+ fs.delete(p1, false);
+
+ Assertions.assertThat(fs.exists(p1))
+ .describedAs("The deleted directory should not exist.")
+ .isFalse();
+ }
+
+ /**
+ * Test that deleting a non-empty explicit directory
+ * can only be done with the recursive flag set to true.
+ *
+ * @throws Exception if an error occurs during the test execution
+ */
+ @Test
+ public void testDeleteNonEmptyExplicitDir() throws Exception {
+ AzureBlobFileSystem fs = getFileSystem();
+
+ Path p1 = new Path("/testDir1");
+ Path p2 = new Path("/testDir2");
+
+ fs.create(new Path("/testDir1/f1.txt"));
+ fs.create(new Path("/testDir2/f2.txt"));
+
+ fs.delete(p1, true);
+
+ //Deleting non-empty dir with recursion set as
+ // false returns a FileAlreadyExistsException: 409-DirectoryNotEmpty
+ intercept(FileAlreadyExistsException.class,
+ () -> fs.delete(p2, false));
+
+ Assertions.assertThat(!fs.exists(p1))
+ .describedAs("FileStatus of the deleted directory should not exist.")
+ .isTrue();
+ Assertions.assertThat(!fs.exists(p1))
Review Comment:
Redundant assert
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -286,6 +304,7 @@ public void testDeleteIdempotencyTriggerHttp404() throws
Exception {
doReturn(idempotencyRetOp).when(mockClient).deleteIdempotencyCheckOp(any());
TracingContext tracingContext = getTestTracingContext(fs, false);
doReturn(tracingContext).when(idempotencyRetOp).createNewTracingContext(any());
+ //To-discuss: If DFS service type, this if case can be removed?
Review Comment:
What is this about?
##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java:
##########
@@ -196,6 +198,22 @@ public void testDeleteIdempotency() throws Exception {
when(op.isARetriedRequest()).thenReturn(true);
// Case 1: Mock instance of Http Operation response. This will return
+ // HTTP:TIMEOUT
+ AbfsHttpOperation http504Op = mock(AbfsHttpOperation.class);
+ when(http504Op.getStatusCode()).thenReturn(HTTP_GATEWAY_TIMEOUT);
+
+ // Mock delete response to 504
+ when(op.getResult()).thenReturn(http504Op);
+ when(op.hasResult()).thenReturn(true);
+
+ Assertions.assertThat(testClient.deleteIdempotencyCheckOp(op)
+ .getResult()
+ .getStatusCode())
+ .describedAs(
Review Comment:
Fix indentation
> ABFS: [FnsOverBlob][Tests] Add Tests For Negative Scenarios Identified for
> Delete Operation
> -------------------------------------------------------------------------------------------
>
> Key: HADOOP-19446
> URL: https://issues.apache.org/jira/browse/HADOOP-19446
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: fs/azure
> Affects Versions: 3.4.1
> Reporter: Anuj Modi
> Assignee: Manika Joshi
> Priority: Major
> Labels: pull-request-available
> Attachments: Delete_TestScenarios.pdf
>
>
> We have identified a few scenarios worth adding integration or mocked
> behavior tests for while implementing FNS Support over Blob Endpoint.
> Attached file shows the the scenarios identified for Ingress Related
> operations on blob Endpoint (Create + Append + Flush filesystem calls)
> This Jira tracks implementing these tests.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
