Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HADOOP-19446. ABFS: [FnsOverBlob][Tests] Add Tests For Negative Scenarios Identified for Delete Operation #7376

Merged
merged 5 commits into from
Mar 12, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1512,8 +1512,24 @@ public AbfsRestOperation deleteBlobPath(final Path blobPath,
final AbfsRestOperation op = getAbfsRestOperation(
AbfsRestOperationType.DeleteBlob, HTTP_METHOD_DELETE, url,
requestHeaders);
op.execute(tracingContext);
return op;
try {
op.execute(tracingContext);
return op;
} catch (AzureBlobFileSystemException e) {
// If we have no HTTP response, throw the original exception.
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;
}
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

"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);
Expand All @@ -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);
Expand Down Expand Up @@ -344,6 +362,123 @@ 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 file1 = new Path("/testDir/dir1/file1");
Path file2 = new Path("/testDir/dir1/file2");
Path implicitDir = file1.getParent();

createAzCopyFile(file1);
createAzCopyFile(file2);

// Deletion of file with different recursion values
fs.delete(file1, false);
fs.delete(file2, true);

Assertions.assertThat(fs.exists(implicitDir))
.describedAs("The directory should exist.")
.isTrue();
Assertions.assertThat(fs.exists(file1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also assert that for file 2 parent will not exist ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parent would still exist right after deletion right? Tested the behaviour through portal as well.

.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();
}

/**
* 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.mkdirs(p1);
fs.mkdirs(p2);
fs.create(new Path("/testDir1/f1.txt"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should first call mkdirs on p1 and p2 for it to exist as explicit dir

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();
}

/**
* Assert that deleting a non-existing path
* returns a false.
*
* @throws Exception if an error occurs during the test execution
*/
@Test
public void testDeleteNonExistingPath() throws Exception {
AzureBlobFileSystem fs = getFileSystem();

Path p = new Path("/nonExistingPath");
Assertions.assertThat(fs.delete(p, true))
.describedAs("Delete operation on non-existing path should return false.")
.isFalse();
}

/**
* Test to check test operation returns false
* 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);

Assertions.assertThat(fs.delete(testFile, true))
.describedAs("Delete operation on deleted path should return false.")
.isFalse();
}

/**
* Tests deleting an implicit directory and its contents. The test verifies that after deletion,
* both the directory and its child file no longer exist.
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this needed?
Also the test name says deleting implicit dir but it seems like it is deleting explicit dir only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the case with implicit path with children and recursion false

// 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")
Expand Down