HADOOP-17765. ABFS: Use Unique File Paths in Tests #3153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
TEST RESULTS HNS Account Location: East US 2 Existing JIRAs to track failures: AppendBlob StreamOps test, LMT, ReadWriteandSeek, testDistCpWithIterator, Lease IO error |
bilaharith
reviewed
Jul 2, 2021
...ls/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java
Outdated
Show resolved
Hide resolved
bilaharith
suggested changes
Jul 2, 2021
steveloughran
requested changes
Jul 9, 2021
There was a problem hiding this comment.
makes sense
- I'd prefer to go with
path()overgetUniquePath()as that's what's been used elsewhere and it'll make copying tests between modules easier. - changes to assertions should take the opportunity to assertPathExists/assertPathDoesNotExist where possible. These are much more useful on test failure
| @@ -443,6 +445,16 @@ protected Path path(String filepath) throws IOException { | |||
| new Path(getTestPath(), filepath)); | |||
| } | |||
|
|
|||
| /** | |||
| * Generate a unique path using the given filepath | |||
| fs.delete(EXISTED_FOLDER_PATH, true); | ||
| assertFalse(fs.exists(EXISTED_FOLDER_PATH)); | ||
| fs.delete(existedFolderPath, true); | ||
| assertFalse(fs.exists(existedFolderPath)); |
There was a problem hiding this comment.
good time to move to the ContractTestUtils assertions assertPathExists/assertPathDoesNotExist as they provide meaningful messages/diagnostics. I really don't like assertTrue/assertFalse without messages, as they make debugging test failures much harder than they need to be
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
TEST RESULTS HNS Account Location: East US 2 Existing JIRAs to track failures: ReadWriteandSeek, testDistCpWithIterator, testAbfsStreamOps, ITestAzureBlobFileSystemLease |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I'm happy with the final set of changes.
While looking at it, I can see that some of the lines you've changed have existing bugs where they don't close the output streams.
Given you are editing these lines, now would be a good time to fix. But: it would be changing more of the code.
Thoughts?
| assertPathDoesNotExist(fs, | ||
| String.format("File with name %s should not exist", createFilePath), | ||
| createFilePath); | ||
| assertTrue(String.format("File with name %s should exist", |
There was a problem hiding this comment.
why switch from AssertPathExists?
If you want to go to the simpler true/false, move to AssertJ whose assert strings "describedAs()" as string formats only evaluated to assert failue.
There was a problem hiding this comment.
test verifies number of fs.exists() method calls based on its use in the assertions
| assertTrue(inputStream.read() != 0); | ||
| } | ||
|
|
||
| // TEST WRITE FILE | ||
| try { | ||
| abfsStore.openFileForWrite(EXISTED_FILE_PATH, fs.getFsStatistics(), true, | ||
| tracingContext); | ||
| abfsStore.openFileForWrite(existedFilePath, fs.getFsStatistics(), true, |
There was a problem hiding this comment.
Looking at this line (and it predates your patch): does the output stream need to be closed?
| assertTrue(fs.exists(EXISTED_FILE_PATH)); | ||
| fs.mkdirs(EXISTED_FOLDER_PATH); | ||
| assertTrue(fs.exists(EXISTED_FOLDER_PATH)); | ||
| fs.create(existedFilePath); |
There was a problem hiding this comment.
should be fs.create(existedFilePath).close(). Not your change, but as the line is changed, time to fix
| @@ -71,7 +70,7 @@ public void testMaxRequestsAndQueueCapacity() throws Exception { | |||
| conf.set(ConfigurationKeys.AZURE_WRITE_MAX_REQUESTS_TO_QUEUE, | |||
| "" + maxRequestsToQueue); | |||
| final AzureBlobFileSystem fs = getFileSystem(conf); | |||
| FSDataOutputStream out = fs.create(TEST_FILE_PATH); | |||
| FSDataOutputStream out = fs.create(path(TEST_FILE_PATH)); | |||
There was a problem hiding this comment.
this should be closed too in try-with-resources or try/finally
|
🎊 +1 overall
This message was automatically generated. |
|
TEST RESULTS HNS Account Location: East US 2 Existing JIRAs to track failures: testDistCp, testAbfsStreamOps, Appendblob lease |
|
🎊 +1 overall
This message was automatically generated. |
|
merged into trunk. Can you cherrypick into branch-3.3 and retest there to make sure all is good. regarding the test failures, #3240 fixes the distcp issues. Please apply and verify that it works on your test setup. |
sumangala-patki
added a commit
to sumangala17/hadoop
that referenced
this pull request
Aug 2, 2021
Contributed by Sumangala Patki (cherry picked from commit 10ba4cc)
kiran-maturi
pushed a commit
to kiran-maturi/hadoop
that referenced
this pull request
Nov 24, 2021
Contributed by Sumangala Patki
asfgit
pushed a commit
that referenced
this pull request
Mar 1, 2022
Contributed by Sumangala Patki Change-Id: Ic8f34bf578069504f7a811a7729982b9c9f49729
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many of ABFS driver tests use common names for file paths (e.g., "/testfile"). This poses a risk of errors during parallel test runs when static variables (such as those for monitoring stats) affected by file paths are introduced.
Using unique test file names will avoid possible errors arising from shared resources during parallel runs.