HADOOP-19445: ABFS: [FnsOverBlob][Tests] Add Tests For Negative Scenarios Identified for Rename Operation #7386
Conversation
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
============================================================
|
|
🎊 +1 overall
This message was automatically generated. |
| @@ -444,15 +443,14 @@ public void createNonRecursivePreCheck(Path parentPath, | |||
| } | |||
| getPathStatus(parentPath.toUri().getPath(), false, | |||
| tracingContext, null); | |||
| incrementAbfsGetPathStatus(); | |||
| } catch (AbfsRestOperationException ex) { | |||
| if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) { | |||
| throw new FileNotFoundException("Cannot create file " | |||
| + parentPath.toUri().getPath() | |||
| + " because parent folder does not exist."); | |||
| } | |||
| throw ex; | |||
There was a problem hiding this comment.
The increase will not get implemented if getPathStatus call fails
There was a problem hiding this comment.
Moved it to finally block
| @@ -993,8 +993,6 @@ public Void call() throws Exception { | |||
| delete(fs.getPath(), fs.isDirectory()); | |||
| if (fs.isDirectory()) { | |||
| statIncrement(DIRECTORIES_DELETED); | |||
| } else { | |||
There was a problem hiding this comment.
should call incrementAbfsDeleteFile ?
There was a problem hiding this comment.
We are incrementing Delete Files in Client.
| /** | ||
| * Increments AbfsCounters for get path status by 1. | ||
| */ | ||
| protected void incrementAbfsGetPathStatus() { |
There was a problem hiding this comment.
counter increment should not be in abfsclient class in my opinion
There was a problem hiding this comment.
As discussed, this is just a placeholder so that we can use the same method in both DFS and Blob client.
| @@ -1751,6 +1749,7 @@ public void takeGetPathStatusAtomicRenameKeyAction(final Path path, | |||
| pendingJsonFileStatus = getPathStatus( | |||
| pendingJsonPath.toUri().getPath(), tracingContext, | |||
| null, false); | |||
| incrementAbfsGetPathStatus(); | |||
There was a problem hiding this comment.
At some places we are incrementing before the operation and at some places after the operation is executed, we must keep it consitent
There was a problem hiding this comment.
Moved this call in finally block everywhere.
| @@ -90,6 +90,7 @@ int getMaxConsumptionParallelism() { | |||
| private boolean deleteInternal(final Path path) | |||
| throws AzureBlobFileSystemException { | |||
| getAbfsClient().deleteBlobPath(path, null, tracingContext); | |||
| getAbfsClient().incrementAbfsDeleteFile(); | |||
There was a problem hiding this comment.
Moved to finally block
|
🎊 +1 overall
This message was automatically generated. |
| @Test(expected = IOException.class) | ||
| public void testRenameBlobToDstWithColonInPath() throws Exception { | ||
| @Test | ||
| public void testRenameBlobToDstWithColonInSourcePath() throws Exception { | ||
| AzureBlobFileSystem fs = getFileSystem(); | ||
| assumeBlobServiceType(); |
There was a problem hiding this comment.
Can we make this run for both DFS and Blob?
There was a problem hiding this comment.
Make sense, removed this assumption.
| AzureBlobFileSystem fs = getFileSystem(); | ||
| assumeBlobServiceType(); | ||
| fs.create(new Path("/src:/file")); | ||
| Assertions.assertThat( |
There was a problem hiding this comment.
Add description for assert
| fs.create(new Path("/src")); | ||
| fs.rename(new Path("/src"), new Path("/dst:file")); | ||
| Assertions.assertThat( |
There was a problem hiding this comment.
Add assert description here and everywhere in file
| * Test the renaming of a directory with different parallelism configurations. | ||
| */ | ||
| @Test | ||
| public void testRenameDirWithDifferentParallelism() throws Exception { |
There was a problem hiding this comment.
Test name should also say its different parallelism configuration not just different parallelsm
| * @throws Exception if any error occurs during the test execution | ||
| */ | ||
| @Test | ||
| public void eTagChangedDuringRename() throws Exception { |
There was a problem hiding this comment.
test name should starte with 'test' here and everywhere
There was a problem hiding this comment.
Added test wherever required.
| @@ -261,7 +269,7 @@ protected String listAndEnqueue(final ListBlobQueue listBlobQueue, | |||
| protected void addPaths(final List<Path> paths, | |||
| final ListResultSchema retrievedSchema) { | |||
| for (ListResultEntrySchema entry : retrievedSchema.paths()) { | |||
| Path entryPath = new Path(ROOT_PATH, entry.name()); | |||
| Path entryPath = new Path(ROOT_PATH + entry.name()); | |||
There was a problem hiding this comment.
As discussed offline, this change is needed because of new Path() behavior in hadoop common.
|
🎊 +1 overall
This message was automatically generated. |
============================================================
|
Jira: https://issues.apache.org/jira/browse/HADOOP-19445
We have identified a few scenarios worth adding integration or mocked behavior tests for while implementing FNS Support over Blob Endpoint. So, we have added test cases for all those scenarios in this PR.