From 6262ad8b1f8e25aeffd762fed8491b5ecc581768 Mon Sep 17 00:00:00 2001 From: Manika Joshi Date: Mon, 10 Feb 2025 06:34:11 -0800 Subject: [PATCH 1/4] Add delete tests for negative scenarios --- .../fs/azurebfs/services/AbfsBlobClient.java | 47 ++++-- .../ITestAzureBlobFileSystemDelete.java | 142 +++++++++++++++++- 2 files changed, 173 insertions(+), 16 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java index b54ce1a4dac7e..7d04c332c2c73 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java @@ -1303,21 +1303,38 @@ public AbfsRestOperation deletePath(final String path, final TracingContext tracingContext) throws AzureBlobFileSystemException { BlobDeleteHandler blobDeleteHandler = getBlobDeleteHandler(path, recursive, tracingContext); - if (blobDeleteHandler.execute()) { - final AbfsUriQueryBuilder abfsUriQueryBuilder - = createDefaultUriQueryBuilder(); - final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString()); - final List requestHeaders = createDefaultHeaders(); - final AbfsRestOperation successOp = getAbfsRestOperation( - AbfsRestOperationType.DeletePath, HTTP_METHOD_DELETE, - url, requestHeaders); - successOp.hardSetResult(HTTP_OK); - return successOp; - } else { - throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR, - AzureServiceErrorCode.UNKNOWN.getErrorCode(), - ERR_DELETE_BLOB + path, - null); + final AbfsUriQueryBuilder abfsUriQueryBuilder + = createDefaultUriQueryBuilder(); + final URL url = createRequestUrl(path, abfsUriQueryBuilder.toString()); + final List requestHeaders = createDefaultHeaders(); + final AbfsRestOperation op = new AbfsRestOperation( + AbfsRestOperationType.DeletePath, this, + HTTP_METHOD_DELETE, url, requestHeaders, getAbfsConfiguration()); + + try { + if (blobDeleteHandler.execute()) { + op.hardSetResult(HttpURLConnection.HTTP_OK); + return op; + } else { + throw new AbfsRestOperationException(HTTP_INTERNAL_ERROR, + AzureServiceErrorCode.UNKNOWN.getErrorCode(), + ERR_DELETE_BLOB + path, + null); + } + } catch (AzureBlobFileSystemException e) { + if (!op.hasResult()) { + throw e; + } + + final AbfsRestOperation idempotencyOp = deleteIdempotencyCheckOp(op); + if (idempotencyOp.getResult().getStatusCode() + == op.getResult().getStatusCode()) { + // idempotency did not return different result + // throw back the exception + throw e; + } else { + return idempotencyOp; + } } } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java index 60bc39979d9d6..5a28ee5a2d25b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java @@ -34,6 +34,7 @@ import org.mockito.Mockito; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileAlreadyExistsException; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -55,6 +56,7 @@ import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_FORBIDDEN; +import static java.net.HttpURLConnection.HTTP_GATEWAY_TIMEOUT; import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import static java.net.HttpURLConnection.HTTP_OK; import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.HTTP_METHOD_DELETE; @@ -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( + "Idempotency check to happen only for HTTP 404 response.") + .isEqualTo(HTTP_GATEWAY_TIMEOUT); + + // Case 2: Mock instance of Http Operation response. This will return // HTTP:Not Found AbfsHttpOperation http404Op = mock(AbfsHttpOperation.class); when(http404Op.getStatusCode()).thenReturn(HTTP_NOT_FOUND); @@ -211,7 +229,7 @@ public void testDeleteIdempotency() throws Exception { "Delete is considered idempotent by default and should return success.") .isEqualTo(HTTP_OK); - // Case 2: Mock instance of Http Operation response. This will return + // Case 3: Mock instance of Http Operation response. This will return // HTTP:Bad Request AbfsHttpOperation http400Op = mock(AbfsHttpOperation.class); when(http400Op.getStatusCode()).thenReturn(HTTP_BAD_REQUEST); @@ -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? if (mockClient instanceof AbfsBlobClient) { doCallRealMethod().when((AbfsBlobClient) mockClient) .getBlobDeleteHandler(Mockito.nullable(String.class), @@ -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)); + } + /** * Tests deleting an implicit directory and its contents. The test verifies that after deletion, * both the directory and its child file no longer exist. @@ -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 + // false returns a FileAlreadyExistsException: 409-DirectoryNotEmpty + intercept(FileAlreadyExistsException.class, + () -> fs.delete(new Path("/testDir/dir1"), false)); fs.delete(new Path("/testDir/dir1"), true); Assertions.assertThat(!fs.exists(new Path("/testDir/dir1"))) .describedAs("FileStatus of the deleted directory should not exist") From ac8d09e7d218b4207249f1630345c68d98bb275d Mon Sep 17 00:00:00 2001 From: Manika Joshi Date: Tue, 25 Feb 2025 22:12:17 -0800 Subject: [PATCH 2/4] addressing comments --- .../fs/azurebfs/ITestAzureBlobFileSystemDelete.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java index 5a28ee5a2d25b..e947e6603527b 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java @@ -207,8 +207,8 @@ public void testDeleteIdempotency() throws Exception { when(op.hasResult()).thenReturn(true); Assertions.assertThat(testClient.deleteIdempotencyCheckOp(op) - .getResult() - .getStatusCode()) + .getResult() + .getStatusCode()) .describedAs( "Idempotency check to happen only for HTTP 404 response.") .isEqualTo(HTTP_GATEWAY_TIMEOUT); @@ -442,9 +442,6 @@ public void testDeleteNonEmptyExplicitDir() throws Exception { 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(); } /** @@ -459,7 +456,7 @@ public void testDeleteNonExistingPath() throws Exception { Path p = new Path("/nonExistingPath"); intercept(FileNotFoundException.class, - () -> assertDeleted(fs, p, true)); + () -> fs.delete(p, true)); } /** @@ -476,7 +473,7 @@ public void testExceptionForDeletedFile() throws Exception { fs.delete(testFile, false); intercept(FileNotFoundException.class, - () -> assertDeleted(fs, testFile, true)); + () -> fs.delete(testFile, true)); } /** From 48bc1af00f181fe44146cf99e1c54e22aa457dcb Mon Sep 17 00:00:00 2001 From: Manika Joshi Date: Tue, 11 Mar 2025 02:15:25 -0700 Subject: [PATCH 3/4] comment suggestions --- .../azurebfs/ITestAzureBlobFileSystemDelete.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java index 9a481c050d7b4..a864f974ee563 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java @@ -304,7 +304,6 @@ 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? if (mockClient instanceof AbfsBlobClient) { doCallRealMethod().when((AbfsBlobClient) mockClient) .getBlobDeleteHandler(Mockito.nullable(String.class), @@ -391,6 +390,9 @@ public void testDeleteFileInImplicitDir() throws Exception { .describedAs("Deleted file should not be present.").isFalse(); Assertions.assertThat(fs.exists(file2)) .describedAs("Deleted file should not be present.").isFalse(); + Assertions.assertThat(fs.exists(implicitDir)) + .describedAs("The parent dir should exist.") + .isTrue(); } /** @@ -426,6 +428,8 @@ public void testDeleteNonEmptyExplicitDir() throws Exception { Path p1 = new Path("/testDir1"); Path p2 = new Path("/testDir2"); + fs.mkdirs(p1); + fs.mkdirs(p2); fs.create(new Path("/testDir1/f1.txt")); fs.create(new Path("/testDir2/f2.txt")); @@ -452,7 +456,9 @@ public void testDeleteNonExistingPath() throws Exception { AzureBlobFileSystem fs = getFileSystem(); Path p = new Path("/nonExistingPath"); - Assertions.assertThat(fs.delete(p, true)).isFalse(); + Assertions.assertThat(fs.delete(p, true)) + .describedAs("Delete operation on non-existing path should return false.") + .isFalse(); } /** @@ -468,7 +474,9 @@ public void testExceptionForDeletedFile() throws Exception { fs.create(testFile); fs.delete(testFile, false); - Assertions.assertThat(fs.delete(testFile, true)).isFalse(); + Assertions.assertThat(fs.delete(testFile, true)) + .describedAs("Delete operation on deleted path should return false.") + .isFalse(); } /** From 7f3e02a058997b6a002ae459b43cfa8038bcb137 Mon Sep 17 00:00:00 2001 From: Manika Joshi Date: Tue, 11 Mar 2025 21:27:28 -0700 Subject: [PATCH 4/4] yetus rerun --- .../hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java index a864f974ee563..3f16e6be80f4a 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemDelete.java @@ -457,7 +457,7 @@ public void testDeleteNonExistingPath() throws Exception { Path p = new Path("/nonExistingPath"); Assertions.assertThat(fs.delete(p, true)) - .describedAs("Delete operation on non-existing path should return false.") + .describedAs("Delete operation on non-existing path should return false") .isFalse(); }