HADOOP-19129: [ABFS] Test Fixes and Test Script Bug Fixes #6676
Conversation
This file contains hidden or 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
|
💔 -1 overall
This message was automatically generated. |
anujmodi2021
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
============================================================
|
|
🎊 +1 overall
This message was automatically generated. |
| logOutput() { | ||
| echo -e "$outputFormatOn" "$1" "$outputFormatOff" | ||
| } | ||
|
|
| expectedListResultsSize, listMaxResults, fileCount) | ||
| .hasSize(expectedListResultsSize); | ||
|
|
||
| AbfsRestOperation op = getFileSystem().getAbfsClient().listPath( |
Contributor
There was a problem hiding this comment.
should we change it to store call to ensure that listing is always complete ?
Contributor
Author
There was a problem hiding this comment.
This test is supposed to test the client behavior only.
We have other tests to test recursive behavior as part of ITestAzureBlobFileSystemListStatus.java
| pos += appendWithOffsetHelper(client, path, data, fs, pos, ONE_MB); | ||
| pos += appendWithOffsetHelper(client, path, data, fs, pos, MB_2); | ||
| appendWithOffsetHelper(client, path, data, fs, pos, MB_4 - 1); | ||
| } |
added 2 commits
April 1, 2024 01:55
anmolanmol1234
approved these changes
Apr 1, 2024
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
============================================================
|
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saxenapranav
suggested changes
Apr 5, 2024
Contributor
saxenapranav
left a comment
saxenapranav
left a comment
There was a problem hiding this comment.
Nice deep dive!
Some comments.
Please add in description example of how the test-script messages are changing.
| assertTrue(String.format("The actual value of %d was not equal to the " | ||
| + "expected value", statistics.getReadOps()), | ||
| statistics.getReadOps() == (largeValue + 3) || statistics.getReadOps() == (largeValue + 4)); | ||
| statistics.getReadOps() >= largeValue || statistics.getReadOps() <= (largeValue + 4)); |
Contributor
There was a problem hiding this comment.
what is reason of these condition changing.
Contributor
Author
There was a problem hiding this comment.
For append blobs data is available to read as soon as it is appended. So in case when data is appended and not yet flushed, it can still be read in append blobs. This can lead to an additional read call for append blobs.
| } | ||
|
|
||
| protected void assumeValidTestConfigPresent(final Configuration conf, final String key) { | ||
| String configuredValue = conf.get(key); |
Contributor
There was a problem hiding this comment.
it should also check conf.get(key., as done in AbfsConfiguration.accountConf(name). All the configs are checked on both key and key.. Dev can give anything.
Contributor
Author
There was a problem hiding this comment.
Makes sense.
Taken
Contributor
There was a problem hiding this comment.
tip: if you set the default to "" then the line below can just check for the string not being empty
Comment on lines
24
to
27
| testresultsregex="Tests run: [0-9]+, Failures: [0-9]+, Errors: [0-9]+, Skipped: [0-9]+$" | ||
| failedTestRegex1="<<< FAILURE!$" | ||
| failedTestRegex2="<<< ERROR!$" | ||
| removeFormattingRegex="s/\x1b\[[0-9;]*m//g" |
Contributor
There was a problem hiding this comment.
Lets add a comment to explain what regex do.
steveloughran
requested changes
Apr 11, 2024
Contributor
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
minor comments, otherwise all good. I'm going to download and test on my now-IOP limited account which does seem to fail in interesting ways
|
|
||
| * [ABFS](./abfs.html) | ||
| * [Testing](./testing_azure.html) | ||
| * [ABFS](./abfs.md) |
Contributor
There was a problem hiding this comment.
-1. when site docs are generated *.md is mapped to *.html.
Contributor
Author
There was a problem hiding this comment.
Reverting this change.
But I observed these links are not working on github.
https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/site/markdown/index.md
They are giving 404
Contributor
There was a problem hiding this comment.
run mvn site:site and you generate the site locally, which is what goes up on apache.org
| } | ||
|
|
||
| protected void assumeValidTestConfigPresent(final Configuration conf, final String key) { | ||
| String configuredValue = conf.get(key); |
Contributor
There was a problem hiding this comment.
tip: if you set the default to "" then the line below can just check for the string not being empty
| expectedListResultsSize, listMaxResults, fileCount) | ||
| .hasSize(expectedListResultsSize); | ||
| } else { | ||
| // Listing is incomplete and number of objects can be lesser than expected |
| // Listing is incomplete and number of objects can be lesser than expected | ||
| Assertions.assertThat(list) | ||
| .describedAs("AbfsClient.listPath() should return %d items" | ||
| + " or lesser when listMaxResults is %d, directory contains" |
| New files created in folder accountSettings is listed in .gitignore to | ||
| prevent accidental cred leaks. | ||
| You are all set to run the test srcipt. |
added 2 commits
April 11, 2024 21:15
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
@anujmodi2021 can you do a PR for this for branch-3.4? |
4 tasks
steveloughran
pushed a commit
that referenced
this pull request
Apr 17, 2024
Contributed by Anuj Modi
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
Apr 19, 2024
Contributed by Anuj Modi
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Description of PR
Test Script used by ABFS to validate changes has following two issues:
Some tests were reported to be failing on OSS trunk including those reported via following Jira:
https://issues.apache.org/jira/browse/HADOOP-19110
https://issues.apache.org/jira/browse/HADOOP-19106
For details of the failing tests and their fixes please refer to the Jira Description:
https://issues.apache.org/jira/browse/HADOOP-19129
How was this patch tested?
Test Suite was run involving different combinations of Auth type and Account type.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?